* [RESEND PATCH v2 0/3] add support to non-uniform SFDP SPI NOR flash memories
@ 2018-08-27 10:26 ` Tudor Ambarus
0 siblings, 0 replies; 22+ messages in thread
From: Tudor Ambarus @ 2018-08-27 10:26 UTC (permalink / raw)
To: marek.vasut, cyrille.pitchen, dwmw2, computersforpeace,
boris.brezillon, richard
Cc: linux-mtd, linux-arm-kernel, linux-kernel, nicolas.ferre,
Cristian.Birsan, Tudor Ambarus
Rebased on spi-nor/next.
Backward compatibility test done on mx25l3273fm2i-08g.
Non-uniform erase test done on sst26vf064b-104i/sn.
Note that in order to do the non-uniform test you'll have to force the
spi_nor_has_uniform_erase() to return false, because the erase map is
tuned to come back to the uniform case. Also, for the SST flashes, you'll
have to unlock the global protection (see https://www.spinics.net/lists/arm-kernel/msg666350.html).
Changes in v2:
- add JESD216B SFDP optional parsers: Sector Map Parameter Table parser and
4-byte Address Instruction Table parser (patch 2/3 and patch 3/3).
- save the Erase Types from bfpt to determine the Erase Type sizes. Sort all
the map's Erase Types in ascending order with the smallest erase size being
the first member in the erase_type array. Sort in the same way the Erase Type
mask in regions in order to have a 1-to-1 correspondence with the Erase
Types defined in the map. We sort the Erase Type by size, at init, in order to
speed up the process of finding the best erase command at run-time.
- remove setting of mtd->erasesize and nor->erase_opcode when parsing the Basic
Flash Parameter Table. mtd->erasesize was set to the maximum supported Erase
Type size, without verifying if that Erase Type can erase the entire flash.
mtd->erasesize is now set just in spi_nor_select_erase(), after we find which
Erase Types are supported in the uniform case.
- spi_nor_select_erase(): pass directly info->sector_size instead of the whole
flash_info structure in order to simplify the code.
- fix condition on finding erase regions:
while (addr < region_start || addr >= region_end) {
- fix the list command adding: list_add_tail() instead of list_add()
- write_enable(nor); before erasing the sectors
- As Boris suggested, remove likely() because it doesn't make any difference
given the time it takes to actually erase the block.
- bool return value for spi_nor_has_uniform_erase().
Changes in v1 or what I've done on top of Cyrille's work (https://lkml.org/lkml/2017/4/15/70):
- minimize the amount of erase() calls by using the best sequence of erase
type commands depending on alignment.
- build the list of best fitted erase commands to be executed once we
validate that the erase can be performed.
- add improvements on how the erase map is handled. The regions are
consecutive in the address space, walk through the regions incrementally.
- speed up finding the best erase type command. Order erase types by
size, iterate them from the biggest to the smallest and stop when best
fitted command is found.
- determine at init if there are erase types that can erase the entire
memory
- fix support for overlaid regions.
Cyrille Pitchen (1):
mtd: spi-nor: parse SFDP 4-byte Address Instruction Table
Tudor Ambarus (2):
mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories
mtd: spi-nor: parse SFDP Sector Map Parameter Table
drivers/mtd/spi-nor/spi-nor.c | 892 +++++++++++++++++++++++++++++++++++++++---
include/linux/mtd/spi-nor.h | 120 ++++++
2 files changed, 954 insertions(+), 58 deletions(-)
--
2.9.4
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RESEND PATCH v2 0/3] add support to non-uniform SFDP SPI NOR flash memories
@ 2018-08-27 10:26 ` Tudor Ambarus
0 siblings, 0 replies; 22+ messages in thread
From: Tudor Ambarus @ 2018-08-27 10:26 UTC (permalink / raw)
To: linux-arm-kernel
Rebased on spi-nor/next.
Backward compatibility test done on mx25l3273fm2i-08g.
Non-uniform erase test done on sst26vf064b-104i/sn.
Note that in order to do the non-uniform test you'll have to force the
spi_nor_has_uniform_erase() to return false, because the erase map is
tuned to come back to the uniform case. Also, for the SST flashes, you'll
have to unlock the global protection (see https://www.spinics.net/lists/arm-kernel/msg666350.html).
Changes in v2:
- add JESD216B SFDP optional parsers: Sector Map Parameter Table parser and
4-byte Address Instruction Table parser (patch 2/3 and patch 3/3).
- save the Erase Types from bfpt to determine the Erase Type sizes. Sort all
the map's Erase Types in ascending order with the smallest erase size being
the first member in the erase_type array. Sort in the same way the Erase Type
mask in regions in order to have a 1-to-1 correspondence with the Erase
Types defined in the map. We sort the Erase Type by size, at init, in order to
speed up the process of finding the best erase command at run-time.
- remove setting of mtd->erasesize and nor->erase_opcode when parsing the Basic
Flash Parameter Table. mtd->erasesize was set to the maximum supported Erase
Type size, without verifying if that Erase Type can erase the entire flash.
mtd->erasesize is now set just in spi_nor_select_erase(), after we find which
Erase Types are supported in the uniform case.
- spi_nor_select_erase(): pass directly info->sector_size instead of the whole
flash_info structure in order to simplify the code.
- fix condition on finding erase regions:
while (addr < region_start || addr >= region_end) {
- fix the list command adding: list_add_tail() instead of list_add()
- write_enable(nor); before erasing the sectors
- As Boris suggested, remove likely() because it doesn't make any difference
given the time it takes to actually erase the block.
- bool return value for spi_nor_has_uniform_erase().
Changes in v1 or what I've done on top of Cyrille's work (https://lkml.org/lkml/2017/4/15/70):
- minimize the amount of erase() calls by using the best sequence of erase
type commands depending on alignment.
- build the list of best fitted erase commands to be executed once we
validate that the erase can be performed.
- add improvements on how the erase map is handled. The regions are
consecutive in the address space, walk through the regions incrementally.
- speed up finding the best erase type command. Order erase types by
size, iterate them from the biggest to the smallest and stop when best
fitted command is found.
- determine at init if there are erase types that can erase the entire
memory
- fix support for overlaid regions.
Cyrille Pitchen (1):
mtd: spi-nor: parse SFDP 4-byte Address Instruction Table
Tudor Ambarus (2):
mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories
mtd: spi-nor: parse SFDP Sector Map Parameter Table
drivers/mtd/spi-nor/spi-nor.c | 892 +++++++++++++++++++++++++++++++++++++++---
include/linux/mtd/spi-nor.h | 120 ++++++
2 files changed, 954 insertions(+), 58 deletions(-)
--
2.9.4
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RESEND PATCH v2 1/3] mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories
2018-08-27 10:26 ` Tudor Ambarus
@ 2018-08-27 10:26 ` Tudor Ambarus
-1 siblings, 0 replies; 22+ messages in thread
From: Tudor Ambarus @ 2018-08-27 10:26 UTC (permalink / raw)
To: marek.vasut, cyrille.pitchen, dwmw2, computersforpeace,
boris.brezillon, richard
Cc: linux-mtd, linux-arm-kernel, linux-kernel, nicolas.ferre,
Cristian.Birsan, Tudor Ambarus
Based on Cyrille Pitchen's patch https://lkml.org/lkml/2017/3/22/935.
This patch is a transitional patch in introducing the support of
SFDP SPI memories with non-uniform erase sizes like Spansion s25fs512s.
Non-uniform erase maps will be used later when initialized based on the
SFDP data.
Introduce the memory erase map which splits the memory array into one
or many erase regions. Each erase region supports up to 4 erase types,
as defined by the JEDEC JESD216B (SFDP) specification.
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.
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 | 475 ++++++++++++++++++++++++++++++++++++++----
include/linux/mtd/spi-nor.h | 109 ++++++++++
2 files changed, 542 insertions(+), 42 deletions(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index f028277..c1e8169 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -18,6 +18,7 @@
#include <linux/math64.h>
#include <linux/sizes.h>
#include <linux/slab.h>
+#include <linux/sort.h>
#include <linux/mtd/mtd.h>
#include <linux/of_platform.h>
@@ -260,6 +261,18 @@ 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_type *erase;
+ int i;
+
+ for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++) {
+ erase = &map->erase_type[i];
+ erase->opcode =
+ spi_nor_convert_3to4_erase(erase->opcode);
+ }
+ }
}
/* Enable/disable 4-byte addressing mode. */
@@ -497,6 +510,206 @@ 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_type *erase,
+ u64 dividend, u32 *remainder)
+{
+ *remainder = (u32)dividend & erase->size_mask;
+ return dividend >> erase->size_shift;
+}
+
+static const struct spi_nor_erase_type *
+spi_nor_find_best_erase_type(const struct spi_nor_erase_map *map,
+ const struct spi_nor_erase_region *region,
+ u64 addr, u32 len)
+{
+ const struct spi_nor_erase_type *erase;
+ u32 rem;
+ int i;
+ u8 erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
+
+ /*
+ * Erase types are ordered by size, with the biggest erase type at
+ * index 0.
+ */
+ for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
+ /* Does the erase region support the tested erase type? */
+ if (!(erase_mask & BIT(i)))
+ continue;
+
+ erase = &map->erase_type[i];
+
+ /* Don't erase more than what the user has asked for. */
+ if (erase->size > len)
+ continue;
+
+ /* Alignment is not mandatory for overlaid regions */
+ if (region->offset & SNOR_OVERLAID_REGION)
+ return erase;
+
+ spi_nor_div_by_erase_size(erase, addr, &rem);
+ if (rem)
+ continue;
+ else
+ return erase;
+ }
+
+ return NULL;
+}
+
+static struct spi_nor_erase_region *
+spi_nor_region_next(struct spi_nor_erase_region *region)
+{
+ if (spi_nor_region_is_last(region))
+ return NULL;
+ return ++region;
+}
+
+static struct spi_nor_erase_region *
+spi_nor_find_erase_region(const struct spi_nor_erase_map *map, u64 addr,
+ u32 len)
+{
+ struct spi_nor_erase_region *region = map->regions;
+ u64 region_start = region->offset & ~SNOR_ERASE_FLAGS_MASK;
+ u64 region_end = region_start + region->size;
+
+ 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 struct spi_nor_erase_command *
+spi_nor_init_erase_cmd(const struct spi_nor_erase_region *region,
+ const struct spi_nor_erase_type *erase, u64 addr)
+{
+ struct spi_nor_erase_command *cmd;
+
+ cmd = kmalloc(sizeof(*cmd), GFP_KERNEL);
+ if (!cmd)
+ return ERR_PTR(-ENOMEM);
+
+ INIT_LIST_HEAD(&cmd->list);
+ cmd->opcode = erase->opcode;
+ cmd->count = 1;
+
+ if (region->offset & SNOR_OVERLAID_REGION)
+ cmd->size = region->size;
+ else
+ cmd->size = erase->size;
+
+ return cmd;
+}
+
+static void spi_nor_destroy_erase_cmd_list(struct list_head *erase_list)
+{
+ struct spi_nor_erase_command *cmd, *next;
+
+ list_for_each_entry_safe(cmd, next, erase_list, list) {
+ list_del(&cmd->list);
+ kfree(cmd);
+ }
+}
+
+static int spi_nor_init_erase_cmd_list(struct spi_nor *nor,
+ struct list_head *erase_list,
+ u64 addr, u32 len)
+{
+ const struct spi_nor_erase_map *map = &nor->erase_map;
+ const struct spi_nor_erase_type *erase, *prev_erase = NULL;
+ struct spi_nor_erase_region *region;
+ struct spi_nor_erase_command *cmd = NULL;
+ u64 region_end;
+ int ret = -EINVAL;
+
+ 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) {
+ erase = spi_nor_find_best_erase_type(map, region, addr, len);
+ if (!erase)
+ goto destroy_erase_cmd_list;
+
+ if (prev_erase != erase ||
+ region->offset & SNOR_OVERLAID_REGION) {
+ cmd = spi_nor_init_erase_cmd(region, erase, addr);
+ if (IS_ERR(cmd)) {
+ ret = PTR_ERR(cmd);
+ goto destroy_erase_cmd_list;
+ }
+
+ list_add_tail(&cmd->list, erase_list);
+ } else {
+ cmd->count++;
+ }
+
+ addr += cmd->size;
+ len -= cmd->size;
+
+ if (len && addr >= region_end) {
+ region = spi_nor_region_next(region);
+ if (!region)
+ goto destroy_erase_cmd_list;
+ region_end = spi_nor_region_end(region);
+ }
+
+ prev_erase = erase;
+ }
+
+ return 0;
+
+destroy_erase_cmd_list:
+ spi_nor_destroy_erase_cmd_list(erase_list);
+ return ret;
+}
+
+static int spi_nor_erase_multi_sectors(struct spi_nor *nor, u64 addr, u32 len)
+{
+ LIST_HEAD(erase_list);
+ struct spi_nor_erase_command *cmd, *next;
+ int ret;
+
+ ret = spi_nor_init_erase_cmd_list(nor, &erase_list, addr, len);
+ if (ret)
+ return ret;
+
+ list_for_each_entry_safe(cmd, next, &erase_list, list) {
+ nor->erase_opcode = cmd->opcode;
+ while (cmd->count) {
+ write_enable(nor);
+
+ ret = spi_nor_erase_sector(nor, addr);
+ if (ret)
+ goto destroy_erase_cmd_list;
+
+ addr += cmd->size;
+ cmd->count--;
+
+ ret = spi_nor_wait_till_ready(nor);
+ if (ret)
+ goto destroy_erase_cmd_list;
+ }
+ list_del(&cmd->list);
+ kfree(cmd);
+ }
+
+ return 0;
+
+destroy_erase_cmd_list:
+ spi_nor_destroy_erase_cmd_list(&erase_list);
+ return ret;
+}
+
/*
* 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 +724,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 (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 +767,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
*/
/* "sector"-at-a-time erase */
- } else {
+ } else if (spi_nor_has_uniform_erase(nor)) {
while (len) {
write_enable(nor);
@@ -567,6 +782,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);
@@ -2165,6 +2386,83 @@ static const struct sfdp_bfpt_erase sfdp_bfpt_erases[] = {
static int spi_nor_hwcaps_read2cmd(u32 hwcaps);
+/* JEDEC JESD216B Standard imposes erase sizes to be power of 2. */
+static inline void
+spi_nor_set_erase_type(struct spi_nor_erase_type *erase, u32 size, u8 opcode)
+{
+ erase->size = size;
+ erase->opcode = opcode;
+ erase->size_shift = ffs(erase->size) - 1;
+ erase->size_mask = (1 << erase->size_shift) - 1;
+}
+
+static inline void
+spi_nor_set_erase_settings_from_bfpt(struct spi_nor_erase_type *erase,
+ u32 size, u8 opcode, u8 i)
+{
+ /*
+ * The supported Erase Types will be sorted at init in ascending order,
+ * with the smallest Erase Type size being the first member
+ * in the erase_type array of the spi_nor_erase_map structure.
+ * Save the Erase Type index as sorted in the Basic Flash Parameter
+ * Table since it will be used later on to synchronize with the
+ * supported Erase Types defined in SFDP optional tables.
+ */
+ erase->idx = i;
+ spi_nor_set_erase_type(erase, size, opcode);
+}
+
+static int spi_nor_cmp_erase_type(const void *a, const void *b)
+{
+ const struct spi_nor_erase_type *erase1 = a;
+ const struct spi_nor_erase_type *erase2 = b;
+
+ return erase1->size - erase2->size;
+}
+
+static void spi_nor_regions_sort_erase_types(struct spi_nor_erase_map *map)
+{
+ struct spi_nor_erase_region *region = map->regions;
+ struct spi_nor_erase_type *erase_type = map->erase_type;
+ int i;
+ u8 region_erase_mask, ordered_erase_mask;
+
+ /*
+ * Sort each region's Erase Types in ascending order with the smallest
+ * Erase Type size starting at BIT(0).
+ */
+ while (region) {
+ region_erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
+
+ /*
+ * The region's erase mask indicates which erase types are
+ * supported from the erase types defined in the map.
+ */
+ ordered_erase_mask = 0;
+ for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++)
+ if (erase_type[i].size &&
+ region_erase_mask & BIT(erase_type[i].idx))
+ ordered_erase_mask |= BIT(i);
+
+ /* Overwrite erase mask. */
+ region->offset = (region->offset & ~SNOR_ERASE_TYPE_MASK) |
+ ordered_erase_mask;
+
+ region = spi_nor_region_next(region);
+ }
+}
+
+static inline void
+spi_nor_init_uniform_erase_map(struct spi_nor_erase_map *map,
+ u8 erase_mask, u64 flash_size)
+{
+ map->uniform_region.offset = SNOR_ERASE_FLAGS_OFFSET(erase_mask, 1, 0,
+ 0);
+ map->uniform_region.size = flash_size;
+ map->regions = &map->uniform_region;
+ map->uniform_erase_type = erase_mask;
+}
+
/**
* spi_nor_parse_bfpt() - read and parse the Basic Flash Parameter Table.
* @nor: pointer to a 'struct spi_nor'
@@ -2199,12 +2497,14 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
const struct sfdp_parameter_header *bfpt_header,
struct spi_nor_flash_parameter *params)
{
- struct mtd_info *mtd = &nor->mtd;
+ struct spi_nor_erase_map *map = &nor->erase_map;
+ struct spi_nor_erase_type *erase_type = map->erase_type;
struct sfdp_bfpt bfpt;
size_t len;
int i, cmd, err;
u32 addr;
u16 half;
+ u8 erase_mask;
/* JESD216 Basic Flash Parameter Table length is at least 9 DWORDs. */
if (bfpt_header->length < BFPT_DWORD_MAX_JESD216)
@@ -2273,7 +2573,12 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
spi_nor_set_read_settings_from_bfpt(read, half, rd->proto);
}
- /* Sector Erase settings. */
+ /*
+ * Sector Erase settings. Reinitialize the uniform erase map using the
+ * Erase Types defined in the bfpt table.
+ */
+ erase_mask = 0;
+ memset(&nor->erase_map, 0, sizeof(nor->erase_map));
for (i = 0; i < ARRAY_SIZE(sfdp_bfpt_erases); i++) {
const struct sfdp_bfpt_erase *er = &sfdp_bfpt_erases[i];
u32 erasesize;
@@ -2288,18 +2593,18 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
erasesize = 1U << erasesize;
opcode = (half >> 8) & 0xff;
-#ifdef CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
- if (erasesize == SZ_4K) {
- nor->erase_opcode = opcode;
- mtd->erasesize = erasesize;
- break;
- }
-#endif
- if (!mtd->erasesize || mtd->erasesize < erasesize) {
- nor->erase_opcode = opcode;
- mtd->erasesize = erasesize;
- }
+ erase_mask |= BIT(i);
+ spi_nor_set_erase_settings_from_bfpt(&erase_type[i], erasesize,
+ opcode, i);
}
+ spi_nor_init_uniform_erase_map(map, erase_mask, params->size);
+ /*
+ * Sort all the map's Erase Types in ascending order with the smallest
+ * erase size being the first member in the erase_type array.
+ */
+ sort(erase_type, SNOR_ERASE_TYPE_MAX, sizeof(erase_type[0]),
+ spi_nor_cmp_erase_type, NULL);
+ spi_nor_regions_sort_erase_types(map);
/* Stop here if not JESD216 rev A or later. */
if (bfpt_header->length < BFPT_DWORD_MAX)
@@ -2455,6 +2760,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 i, erase_mask;
+
/* Set legacy flash parameters as default. */
memset(params, 0, sizeof(*params));
@@ -2494,6 +2802,28 @@ 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. Sort Erase Types in ascending order, with the
+ * smallest erase size starting at BIT(0).
+ */
+ erase_mask = 0;
+ i = 0;
+ if (info->flags & SECT_4K_PMC) {
+ erase_mask |= BIT(i);
+ spi_nor_set_erase_type(&map->erase_type[i], 4096u,
+ SPINOR_OP_BE_4K_PMC);
+ i++;
+ } else if (info->flags & SECT_4K) {
+ erase_mask |= BIT(i);
+ spi_nor_set_erase_type(&map->erase_type[i], 4096u,
+ SPINOR_OP_BE_4K);
+ i++;
+ }
+ erase_mask |= BIT(i);
+ spi_nor_set_erase_type(&map->erase_type[i], info->sector_size,
+ SPINOR_OP_SE);
+ 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)) {
@@ -2521,20 +2851,20 @@ static int spi_nor_init_params(struct spi_nor *nor,
params->quad_enable = info->quad_enable;
}
- /* Override the parameters with data read from SFDP tables. */
- nor->addr_width = 0;
- nor->mtd.erasesize = 0;
if ((info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
!(info->flags & SPI_NOR_SKIP_SFDP)) {
struct spi_nor_flash_parameter sfdp_params;
+ struct spi_nor_erase_map prev_map;
memcpy(&sfdp_params, params, sizeof(sfdp_params));
- if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
- nor->addr_width = 0;
- nor->mtd.erasesize = 0;
- } else {
+ memcpy(&prev_map, &nor->erase_map, sizeof(prev_map));
+
+ if (spi_nor_parse_sfdp(nor, &sfdp_params))
+ /* restore previous erase map */
+ memcpy(&nor->erase_map, &prev_map,
+ sizeof(nor->erase_map));
+ else
memcpy(params, &sfdp_params, sizeof(*params));
- }
}
return 0;
@@ -2643,29 +2973,90 @@ static int spi_nor_select_pp(struct spi_nor *nor,
return 0;
}
-static int spi_nor_select_erase(struct spi_nor *nor,
- const struct flash_info *info)
+static const struct spi_nor_erase_type *
+spi_nor_select_uniform_erase(struct spi_nor_erase_map *map,
+ const u32 sector_size)
{
- struct mtd_info *mtd = &nor->mtd;
+ const struct spi_nor_erase_type *tested_erase, *erase = NULL;
+ int i;
+ u8 uniform_erase_type = map->uniform_erase_type;
- /* Do nothing if already configured from SFDP. */
- if (mtd->erasesize)
- return 0;
+ for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
+ if (!(uniform_erase_type & BIT(i)))
+ continue;
+
+ tested_erase = &map->erase_type[i];
+
+ /*
+ * If the current erase size is the one, stop here:
+ * we have found the right uniform Sector Erase command.
+ */
+ if (tested_erase->size == sector_size) {
+ erase = tested_erase;
+ break;
+ }
+
+ /*
+ * Otherwise, the current erase size is still a valid canditate.
+ * Select the biggest valid candidate.
+ */
+ if (!erase && tested_erase->size)
+ erase = tested_erase;
+ }
+ if (!erase)
+ return ERR_PTR(-EINVAL);
+
+ /* Disable all other Sector Erase commands. */
+ map->uniform_erase_type &= ~SNOR_ERASE_TYPE_MASK;
+ map->uniform_erase_type |= BIT(erase - map->erase_type);
+ return erase;
+}
+
+static int spi_nor_select_erase(struct spi_nor *nor, u32 sector_size)
+{
+ struct spi_nor_erase_map *map = &nor->erase_map;
+ const struct spi_nor_erase_type *erase = NULL;
+ struct mtd_info *mtd = &nor->mtd;
+ int i;
+
+ /*
+ * 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
+ sector_size = 4096u;
#endif
- {
- nor->erase_opcode = SPINOR_OP_SE;
- mtd->erasesize = info->sector_size;
+
+ if (spi_nor_has_uniform_erase(nor)) {
+ erase = spi_nor_select_uniform_erase(map, sector_size);
+ if (IS_ERR(erase))
+ return PTR_ERR(erase);
+ nor->erase_opcode = erase->opcode;
+ mtd->erasesize = erase->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 = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
+ if (map->erase_type[i].size) {
+ erase = &map->erase_type[i];
+ break;
+ }
+ }
+
+ if (!erase)
+ return -EINVAL;
+
+ mtd->erasesize = erase->size;
return 0;
}
@@ -2712,7 +3103,7 @@ static int spi_nor_setup(struct spi_nor *nor, const struct flash_info *info,
}
/* Select the Sector Erase command. */
- err = spi_nor_select_erase(nor, info);
+ err = spi_nor_select_erase(nor, info->sector_size);
if (err) {
dev_err(nor->dev,
"can't select erase settings supported by both the SPI controller and memory.\n");
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index c922e97..5be9d20 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -239,6 +239,101 @@ enum spi_nor_option_flags {
};
/**
+ * struct spi_nor_erase_type - Structure to describe a SPI NOR erase type
+ * @size: the size of the sector/block erased by the erase type.
+ * 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.
+ * @idx: Erase Type index as sorted in the Basic Flash Parameter
+ * Table. It will be used to synchronize the supported
+ * Erase Types with the ones identified in the SFDP
+ * optional tables.
+ */
+struct spi_nor_erase_type {
+ u32 size;
+ u32 size_shift;
+ u32 size_mask;
+ u8 opcode;
+ u8 idx;
+};
+
+/**
+ * struct spi_nor_erase_command - Used for non-uniform erases
+ * The structure is used to describe a list of erase commands to be executed
+ * once we validate that the erase can be performed. The elements in the list
+ * are run-length encoded.
+ * @list: for inclusion into the list of erase commands.
+ * @count: how many times the same erase command should be
+ * consecutively used.
+ * @size: the size of the sector/block erased by the command.
+ * @opcode: the SPI command op code to erase the sector/block.
+ */
+struct spi_nor_erase_command {
+ struct list_head list;
+ u32 count;
+ u32 size;
+ 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 sorted in ascending order with the
+ * smallest Erase Type size being at BIT(0).
+ * @size: the size of the region in bytes.
+ */
+struct spi_nor_erase_region {
+ u64 offset;
+ u64 size;
+};
+
+#define SNOR_ERASE_TYPE_MAX 4
+#define SNOR_ERASE_TYPE_MASK GENMASK_ULL(SNOR_ERASE_TYPE_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_ERASE_TYPE_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: array of erase regions. The regions are consecutive in
+ * address space. Walking through the regions is done
+ * incrementally.
+ * @uniform_region: a pre-allocated erase region for SPI NOR with a uniform
+ * sector size (legacy implementation).
+ * @erase_type: an array of erase types shared by all the regions.
+ * The erase types are sorted in ascending order, with the
+ * smallest Erase Type size being the first member in the
+ * erase_type array.
+ * @uniform_erase_type: bitmask encoding erase types that can erase the
+ * entire memory. This member is completed at init by
+ * uniform and non-uniform SPI NOR flash memories if they
+ * support at least one erase type 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_type erase_type[SNOR_ERASE_TYPE_MAX];
+ u8 uniform_erase_type;
+};
+
+/**
* struct flash_info - Forward declaration of a structure used internally by
* spi_nor_scan()
*/
@@ -262,6 +357,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
@@ -297,6 +393,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);
@@ -317,6 +414,18 @@ 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 bool spi_nor_has_uniform_erase(const struct spi_nor *nor)
+{
+ return !!nor->erase_map.uniform_erase_type;
+}
+
static inline void spi_nor_set_flash_node(struct spi_nor *nor,
struct device_node *np)
{
--
2.9.4
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [RESEND PATCH v2 1/3] mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories
@ 2018-08-27 10:26 ` Tudor Ambarus
0 siblings, 0 replies; 22+ messages in thread
From: Tudor Ambarus @ 2018-08-27 10:26 UTC (permalink / raw)
To: linux-arm-kernel
Based on Cyrille Pitchen's patch https://lkml.org/lkml/2017/3/22/935.
This patch is a transitional patch in introducing the support of
SFDP SPI memories with non-uniform erase sizes like Spansion s25fs512s.
Non-uniform erase maps will be used later when initialized based on the
SFDP data.
Introduce the memory erase map which splits the memory array into one
or many erase regions. Each erase region supports up to 4 erase types,
as defined by the JEDEC JESD216B (SFDP) specification.
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.
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 | 475 ++++++++++++++++++++++++++++++++++++++----
include/linux/mtd/spi-nor.h | 109 ++++++++++
2 files changed, 542 insertions(+), 42 deletions(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index f028277..c1e8169 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -18,6 +18,7 @@
#include <linux/math64.h>
#include <linux/sizes.h>
#include <linux/slab.h>
+#include <linux/sort.h>
#include <linux/mtd/mtd.h>
#include <linux/of_platform.h>
@@ -260,6 +261,18 @@ 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_type *erase;
+ int i;
+
+ for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++) {
+ erase = &map->erase_type[i];
+ erase->opcode =
+ spi_nor_convert_3to4_erase(erase->opcode);
+ }
+ }
}
/* Enable/disable 4-byte addressing mode. */
@@ -497,6 +510,206 @@ 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_type *erase,
+ u64 dividend, u32 *remainder)
+{
+ *remainder = (u32)dividend & erase->size_mask;
+ return dividend >> erase->size_shift;
+}
+
+static const struct spi_nor_erase_type *
+spi_nor_find_best_erase_type(const struct spi_nor_erase_map *map,
+ const struct spi_nor_erase_region *region,
+ u64 addr, u32 len)
+{
+ const struct spi_nor_erase_type *erase;
+ u32 rem;
+ int i;
+ u8 erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
+
+ /*
+ * Erase types are ordered by size, with the biggest erase type at
+ * index 0.
+ */
+ for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
+ /* Does the erase region support the tested erase type? */
+ if (!(erase_mask & BIT(i)))
+ continue;
+
+ erase = &map->erase_type[i];
+
+ /* Don't erase more than what the user has asked for. */
+ if (erase->size > len)
+ continue;
+
+ /* Alignment is not mandatory for overlaid regions */
+ if (region->offset & SNOR_OVERLAID_REGION)
+ return erase;
+
+ spi_nor_div_by_erase_size(erase, addr, &rem);
+ if (rem)
+ continue;
+ else
+ return erase;
+ }
+
+ return NULL;
+}
+
+static struct spi_nor_erase_region *
+spi_nor_region_next(struct spi_nor_erase_region *region)
+{
+ if (spi_nor_region_is_last(region))
+ return NULL;
+ return ++region;
+}
+
+static struct spi_nor_erase_region *
+spi_nor_find_erase_region(const struct spi_nor_erase_map *map, u64 addr,
+ u32 len)
+{
+ struct spi_nor_erase_region *region = map->regions;
+ u64 region_start = region->offset & ~SNOR_ERASE_FLAGS_MASK;
+ u64 region_end = region_start + region->size;
+
+ 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 struct spi_nor_erase_command *
+spi_nor_init_erase_cmd(const struct spi_nor_erase_region *region,
+ const struct spi_nor_erase_type *erase, u64 addr)
+{
+ struct spi_nor_erase_command *cmd;
+
+ cmd = kmalloc(sizeof(*cmd), GFP_KERNEL);
+ if (!cmd)
+ return ERR_PTR(-ENOMEM);
+
+ INIT_LIST_HEAD(&cmd->list);
+ cmd->opcode = erase->opcode;
+ cmd->count = 1;
+
+ if (region->offset & SNOR_OVERLAID_REGION)
+ cmd->size = region->size;
+ else
+ cmd->size = erase->size;
+
+ return cmd;
+}
+
+static void spi_nor_destroy_erase_cmd_list(struct list_head *erase_list)
+{
+ struct spi_nor_erase_command *cmd, *next;
+
+ list_for_each_entry_safe(cmd, next, erase_list, list) {
+ list_del(&cmd->list);
+ kfree(cmd);
+ }
+}
+
+static int spi_nor_init_erase_cmd_list(struct spi_nor *nor,
+ struct list_head *erase_list,
+ u64 addr, u32 len)
+{
+ const struct spi_nor_erase_map *map = &nor->erase_map;
+ const struct spi_nor_erase_type *erase, *prev_erase = NULL;
+ struct spi_nor_erase_region *region;
+ struct spi_nor_erase_command *cmd = NULL;
+ u64 region_end;
+ int ret = -EINVAL;
+
+ 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) {
+ erase = spi_nor_find_best_erase_type(map, region, addr, len);
+ if (!erase)
+ goto destroy_erase_cmd_list;
+
+ if (prev_erase != erase ||
+ region->offset & SNOR_OVERLAID_REGION) {
+ cmd = spi_nor_init_erase_cmd(region, erase, addr);
+ if (IS_ERR(cmd)) {
+ ret = PTR_ERR(cmd);
+ goto destroy_erase_cmd_list;
+ }
+
+ list_add_tail(&cmd->list, erase_list);
+ } else {
+ cmd->count++;
+ }
+
+ addr += cmd->size;
+ len -= cmd->size;
+
+ if (len && addr >= region_end) {
+ region = spi_nor_region_next(region);
+ if (!region)
+ goto destroy_erase_cmd_list;
+ region_end = spi_nor_region_end(region);
+ }
+
+ prev_erase = erase;
+ }
+
+ return 0;
+
+destroy_erase_cmd_list:
+ spi_nor_destroy_erase_cmd_list(erase_list);
+ return ret;
+}
+
+static int spi_nor_erase_multi_sectors(struct spi_nor *nor, u64 addr, u32 len)
+{
+ LIST_HEAD(erase_list);
+ struct spi_nor_erase_command *cmd, *next;
+ int ret;
+
+ ret = spi_nor_init_erase_cmd_list(nor, &erase_list, addr, len);
+ if (ret)
+ return ret;
+
+ list_for_each_entry_safe(cmd, next, &erase_list, list) {
+ nor->erase_opcode = cmd->opcode;
+ while (cmd->count) {
+ write_enable(nor);
+
+ ret = spi_nor_erase_sector(nor, addr);
+ if (ret)
+ goto destroy_erase_cmd_list;
+
+ addr += cmd->size;
+ cmd->count--;
+
+ ret = spi_nor_wait_till_ready(nor);
+ if (ret)
+ goto destroy_erase_cmd_list;
+ }
+ list_del(&cmd->list);
+ kfree(cmd);
+ }
+
+ return 0;
+
+destroy_erase_cmd_list:
+ spi_nor_destroy_erase_cmd_list(&erase_list);
+ return ret;
+}
+
/*
* 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 +724,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 (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 +767,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
*/
/* "sector"-at-a-time erase */
- } else {
+ } else if (spi_nor_has_uniform_erase(nor)) {
while (len) {
write_enable(nor);
@@ -567,6 +782,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);
@@ -2165,6 +2386,83 @@ static const struct sfdp_bfpt_erase sfdp_bfpt_erases[] = {
static int spi_nor_hwcaps_read2cmd(u32 hwcaps);
+/* JEDEC JESD216B Standard imposes erase sizes to be power of 2. */
+static inline void
+spi_nor_set_erase_type(struct spi_nor_erase_type *erase, u32 size, u8 opcode)
+{
+ erase->size = size;
+ erase->opcode = opcode;
+ erase->size_shift = ffs(erase->size) - 1;
+ erase->size_mask = (1 << erase->size_shift) - 1;
+}
+
+static inline void
+spi_nor_set_erase_settings_from_bfpt(struct spi_nor_erase_type *erase,
+ u32 size, u8 opcode, u8 i)
+{
+ /*
+ * The supported Erase Types will be sorted at init in ascending order,
+ * with the smallest Erase Type size being the first member
+ * in the erase_type array of the spi_nor_erase_map structure.
+ * Save the Erase Type index as sorted in the Basic Flash Parameter
+ * Table since it will be used later on to synchronize with the
+ * supported Erase Types defined in SFDP optional tables.
+ */
+ erase->idx = i;
+ spi_nor_set_erase_type(erase, size, opcode);
+}
+
+static int spi_nor_cmp_erase_type(const void *a, const void *b)
+{
+ const struct spi_nor_erase_type *erase1 = a;
+ const struct spi_nor_erase_type *erase2 = b;
+
+ return erase1->size - erase2->size;
+}
+
+static void spi_nor_regions_sort_erase_types(struct spi_nor_erase_map *map)
+{
+ struct spi_nor_erase_region *region = map->regions;
+ struct spi_nor_erase_type *erase_type = map->erase_type;
+ int i;
+ u8 region_erase_mask, ordered_erase_mask;
+
+ /*
+ * Sort each region's Erase Types in ascending order with the smallest
+ * Erase Type size starting at BIT(0).
+ */
+ while (region) {
+ region_erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
+
+ /*
+ * The region's erase mask indicates which erase types are
+ * supported from the erase types defined in the map.
+ */
+ ordered_erase_mask = 0;
+ for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++)
+ if (erase_type[i].size &&
+ region_erase_mask & BIT(erase_type[i].idx))
+ ordered_erase_mask |= BIT(i);
+
+ /* Overwrite erase mask. */
+ region->offset = (region->offset & ~SNOR_ERASE_TYPE_MASK) |
+ ordered_erase_mask;
+
+ region = spi_nor_region_next(region);
+ }
+}
+
+static inline void
+spi_nor_init_uniform_erase_map(struct spi_nor_erase_map *map,
+ u8 erase_mask, u64 flash_size)
+{
+ map->uniform_region.offset = SNOR_ERASE_FLAGS_OFFSET(erase_mask, 1, 0,
+ 0);
+ map->uniform_region.size = flash_size;
+ map->regions = &map->uniform_region;
+ map->uniform_erase_type = erase_mask;
+}
+
/**
* spi_nor_parse_bfpt() - read and parse the Basic Flash Parameter Table.
* @nor: pointer to a 'struct spi_nor'
@@ -2199,12 +2497,14 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
const struct sfdp_parameter_header *bfpt_header,
struct spi_nor_flash_parameter *params)
{
- struct mtd_info *mtd = &nor->mtd;
+ struct spi_nor_erase_map *map = &nor->erase_map;
+ struct spi_nor_erase_type *erase_type = map->erase_type;
struct sfdp_bfpt bfpt;
size_t len;
int i, cmd, err;
u32 addr;
u16 half;
+ u8 erase_mask;
/* JESD216 Basic Flash Parameter Table length is at least 9 DWORDs. */
if (bfpt_header->length < BFPT_DWORD_MAX_JESD216)
@@ -2273,7 +2573,12 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
spi_nor_set_read_settings_from_bfpt(read, half, rd->proto);
}
- /* Sector Erase settings. */
+ /*
+ * Sector Erase settings. Reinitialize the uniform erase map using the
+ * Erase Types defined in the bfpt table.
+ */
+ erase_mask = 0;
+ memset(&nor->erase_map, 0, sizeof(nor->erase_map));
for (i = 0; i < ARRAY_SIZE(sfdp_bfpt_erases); i++) {
const struct sfdp_bfpt_erase *er = &sfdp_bfpt_erases[i];
u32 erasesize;
@@ -2288,18 +2593,18 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
erasesize = 1U << erasesize;
opcode = (half >> 8) & 0xff;
-#ifdef CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
- if (erasesize == SZ_4K) {
- nor->erase_opcode = opcode;
- mtd->erasesize = erasesize;
- break;
- }
-#endif
- if (!mtd->erasesize || mtd->erasesize < erasesize) {
- nor->erase_opcode = opcode;
- mtd->erasesize = erasesize;
- }
+ erase_mask |= BIT(i);
+ spi_nor_set_erase_settings_from_bfpt(&erase_type[i], erasesize,
+ opcode, i);
}
+ spi_nor_init_uniform_erase_map(map, erase_mask, params->size);
+ /*
+ * Sort all the map's Erase Types in ascending order with the smallest
+ * erase size being the first member in the erase_type array.
+ */
+ sort(erase_type, SNOR_ERASE_TYPE_MAX, sizeof(erase_type[0]),
+ spi_nor_cmp_erase_type, NULL);
+ spi_nor_regions_sort_erase_types(map);
/* Stop here if not JESD216 rev A or later. */
if (bfpt_header->length < BFPT_DWORD_MAX)
@@ -2455,6 +2760,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 i, erase_mask;
+
/* Set legacy flash parameters as default. */
memset(params, 0, sizeof(*params));
@@ -2494,6 +2802,28 @@ 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. Sort Erase Types in ascending order, with the
+ * smallest erase size starting at BIT(0).
+ */
+ erase_mask = 0;
+ i = 0;
+ if (info->flags & SECT_4K_PMC) {
+ erase_mask |= BIT(i);
+ spi_nor_set_erase_type(&map->erase_type[i], 4096u,
+ SPINOR_OP_BE_4K_PMC);
+ i++;
+ } else if (info->flags & SECT_4K) {
+ erase_mask |= BIT(i);
+ spi_nor_set_erase_type(&map->erase_type[i], 4096u,
+ SPINOR_OP_BE_4K);
+ i++;
+ }
+ erase_mask |= BIT(i);
+ spi_nor_set_erase_type(&map->erase_type[i], info->sector_size,
+ SPINOR_OP_SE);
+ 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)) {
@@ -2521,20 +2851,20 @@ static int spi_nor_init_params(struct spi_nor *nor,
params->quad_enable = info->quad_enable;
}
- /* Override the parameters with data read from SFDP tables. */
- nor->addr_width = 0;
- nor->mtd.erasesize = 0;
if ((info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
!(info->flags & SPI_NOR_SKIP_SFDP)) {
struct spi_nor_flash_parameter sfdp_params;
+ struct spi_nor_erase_map prev_map;
memcpy(&sfdp_params, params, sizeof(sfdp_params));
- if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
- nor->addr_width = 0;
- nor->mtd.erasesize = 0;
- } else {
+ memcpy(&prev_map, &nor->erase_map, sizeof(prev_map));
+
+ if (spi_nor_parse_sfdp(nor, &sfdp_params))
+ /* restore previous erase map */
+ memcpy(&nor->erase_map, &prev_map,
+ sizeof(nor->erase_map));
+ else
memcpy(params, &sfdp_params, sizeof(*params));
- }
}
return 0;
@@ -2643,29 +2973,90 @@ static int spi_nor_select_pp(struct spi_nor *nor,
return 0;
}
-static int spi_nor_select_erase(struct spi_nor *nor,
- const struct flash_info *info)
+static const struct spi_nor_erase_type *
+spi_nor_select_uniform_erase(struct spi_nor_erase_map *map,
+ const u32 sector_size)
{
- struct mtd_info *mtd = &nor->mtd;
+ const struct spi_nor_erase_type *tested_erase, *erase = NULL;
+ int i;
+ u8 uniform_erase_type = map->uniform_erase_type;
- /* Do nothing if already configured from SFDP. */
- if (mtd->erasesize)
- return 0;
+ for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
+ if (!(uniform_erase_type & BIT(i)))
+ continue;
+
+ tested_erase = &map->erase_type[i];
+
+ /*
+ * If the current erase size is the one, stop here:
+ * we have found the right uniform Sector Erase command.
+ */
+ if (tested_erase->size == sector_size) {
+ erase = tested_erase;
+ break;
+ }
+
+ /*
+ * Otherwise, the current erase size is still a valid canditate.
+ * Select the biggest valid candidate.
+ */
+ if (!erase && tested_erase->size)
+ erase = tested_erase;
+ }
+ if (!erase)
+ return ERR_PTR(-EINVAL);
+
+ /* Disable all other Sector Erase commands. */
+ map->uniform_erase_type &= ~SNOR_ERASE_TYPE_MASK;
+ map->uniform_erase_type |= BIT(erase - map->erase_type);
+ return erase;
+}
+
+static int spi_nor_select_erase(struct spi_nor *nor, u32 sector_size)
+{
+ struct spi_nor_erase_map *map = &nor->erase_map;
+ const struct spi_nor_erase_type *erase = NULL;
+ struct mtd_info *mtd = &nor->mtd;
+ int i;
+
+ /*
+ * 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
+ sector_size = 4096u;
#endif
- {
- nor->erase_opcode = SPINOR_OP_SE;
- mtd->erasesize = info->sector_size;
+
+ if (spi_nor_has_uniform_erase(nor)) {
+ erase = spi_nor_select_uniform_erase(map, sector_size);
+ if (IS_ERR(erase))
+ return PTR_ERR(erase);
+ nor->erase_opcode = erase->opcode;
+ mtd->erasesize = erase->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 = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
+ if (map->erase_type[i].size) {
+ erase = &map->erase_type[i];
+ break;
+ }
+ }
+
+ if (!erase)
+ return -EINVAL;
+
+ mtd->erasesize = erase->size;
return 0;
}
@@ -2712,7 +3103,7 @@ static int spi_nor_setup(struct spi_nor *nor, const struct flash_info *info,
}
/* Select the Sector Erase command. */
- err = spi_nor_select_erase(nor, info);
+ err = spi_nor_select_erase(nor, info->sector_size);
if (err) {
dev_err(nor->dev,
"can't select erase settings supported by both the SPI controller and memory.\n");
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index c922e97..5be9d20 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -239,6 +239,101 @@ enum spi_nor_option_flags {
};
/**
+ * struct spi_nor_erase_type - Structure to describe a SPI NOR erase type
+ * @size: the size of the sector/block erased by the erase type.
+ * 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.
+ * @idx: Erase Type index as sorted in the Basic Flash Parameter
+ * Table. It will be used to synchronize the supported
+ * Erase Types with the ones identified in the SFDP
+ * optional tables.
+ */
+struct spi_nor_erase_type {
+ u32 size;
+ u32 size_shift;
+ u32 size_mask;
+ u8 opcode;
+ u8 idx;
+};
+
+/**
+ * struct spi_nor_erase_command - Used for non-uniform erases
+ * The structure is used to describe a list of erase commands to be executed
+ * once we validate that the erase can be performed. The elements in the list
+ * are run-length encoded.
+ * @list: for inclusion into the list of erase commands.
+ * @count: how many times the same erase command should be
+ * consecutively used.
+ * @size: the size of the sector/block erased by the command.
+ * @opcode: the SPI command op code to erase the sector/block.
+ */
+struct spi_nor_erase_command {
+ struct list_head list;
+ u32 count;
+ u32 size;
+ 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 sorted in ascending order with the
+ * smallest Erase Type size being at BIT(0).
+ * @size: the size of the region in bytes.
+ */
+struct spi_nor_erase_region {
+ u64 offset;
+ u64 size;
+};
+
+#define SNOR_ERASE_TYPE_MAX 4
+#define SNOR_ERASE_TYPE_MASK GENMASK_ULL(SNOR_ERASE_TYPE_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_ERASE_TYPE_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: array of erase regions. The regions are consecutive in
+ * address space. Walking through the regions is done
+ * incrementally.
+ * @uniform_region: a pre-allocated erase region for SPI NOR with a uniform
+ * sector size (legacy implementation).
+ * @erase_type: an array of erase types shared by all the regions.
+ * The erase types are sorted in ascending order, with the
+ * smallest Erase Type size being the first member in the
+ * erase_type array.
+ * @uniform_erase_type: bitmask encoding erase types that can erase the
+ * entire memory. This member is completed at init by
+ * uniform and non-uniform SPI NOR flash memories if they
+ * support at least one erase type 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_type erase_type[SNOR_ERASE_TYPE_MAX];
+ u8 uniform_erase_type;
+};
+
+/**
* struct flash_info - Forward declaration of a structure used internally by
* spi_nor_scan()
*/
@@ -262,6 +357,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
@@ -297,6 +393,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);
@@ -317,6 +414,18 @@ 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 bool spi_nor_has_uniform_erase(const struct spi_nor *nor)
+{
+ return !!nor->erase_map.uniform_erase_type;
+}
+
static inline void spi_nor_set_flash_node(struct spi_nor *nor,
struct device_node *np)
{
--
2.9.4
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [RESEND PATCH v2 2/3] mtd: spi-nor: parse SFDP Sector Map Parameter Table
2018-08-27 10:26 ` Tudor Ambarus
@ 2018-08-27 10:26 ` Tudor Ambarus
-1 siblings, 0 replies; 22+ messages in thread
From: Tudor Ambarus @ 2018-08-27 10:26 UTC (permalink / raw)
To: marek.vasut, cyrille.pitchen, dwmw2, computersforpeace,
boris.brezillon, richard
Cc: linux-mtd, linux-arm-kernel, linux-kernel, nicolas.ferre,
Cristian.Birsan, Tudor Ambarus
Add support for the SFDP (JESD216B) Sector Map Parameter Table. This
table is optional, but when available, we parse it to identify the
location and size of sectors within the main data array of the
flash memory device and to identify which Erase Types are supported by
each sector.
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
drivers/mtd/spi-nor/spi-nor.c | 269 +++++++++++++++++++++++++++++++++++++++---
include/linux/mtd/spi-nor.h | 11 ++
2 files changed, 264 insertions(+), 16 deletions(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index c1e8169..522d5aa 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2085,6 +2085,35 @@ spi_nor_set_pp_settings(struct spi_nor_pp_command *pp,
*/
/**
+ * spi_nor_read_raw() - raw read of serial flash memory. read_opcode,
+ * addr_width and read_dummy members of the struct spi_nor should be previously
+ * set.
+ * @nor: pointer to a 'struct spi_nor'
+ * @addr: offset in the serial flash memory
+ * @len: number of bytes to read
+ * @buf: buffer where the data is copied into
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spi_nor_read_raw(struct spi_nor *nor, u32 addr, size_t len, u8 *buf)
+{
+ int ret;
+
+ while (len) {
+ ret = nor->read(nor, addr, len, buf);
+ if (!ret || ret > len)
+ return -EIO;
+ if (ret < 0)
+ return ret;
+
+ buf += ret;
+ addr += ret;
+ len -= ret;
+ }
+ return 0;
+}
+
+/**
* spi_nor_read_sfdp() - read Serial Flash Discoverable Parameters.
* @nor: pointer to a 'struct spi_nor'
* @addr: offset in the SFDP area to start reading data from
@@ -2111,22 +2140,8 @@ static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
nor->addr_width = 3;
nor->read_dummy = 8;
- while (len) {
- ret = nor->read(nor, addr, len, (u8 *)buf);
- if (!ret || ret > len) {
- ret = -EIO;
- goto read_err;
- }
- if (ret < 0)
- goto read_err;
+ ret = spi_nor_read_raw(nor, addr, len, buf);
- buf += ret;
- addr += ret;
- len -= ret;
- }
- ret = 0;
-
-read_err:
nor->read_opcode = read_opcode;
nor->addr_width = addr_width;
nor->read_dummy = read_dummy;
@@ -2646,6 +2661,228 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
return 0;
}
+#define SMPT_CMD_ADDRESS_LEN_MASK GENMASK(23, 22)
+#define SMPT_CMD_ADDRESS_LEN_0 (0x0UL << 22)
+#define SMPT_CMD_ADDRESS_LEN_3 (0x1UL << 22)
+#define SMPT_CMD_ADDRESS_LEN_4 (0x2UL << 22)
+#define SMPT_CMD_ADDRESS_LEN_USE_CURRENT (0x3UL << 22)
+
+#define SMPT_CMD_READ_DUMMY_MASK GENMASK(19, 16)
+#define SMPT_CMD_READ_DUMMY_SHIFT 16
+#define SMPT_CMD_READ_DUMMY(_cmd) \
+ ((_cmd & SMPT_CMD_READ_DUMMY_MASK) >> SMPT_CMD_READ_DUMMY_SHIFT)
+#define SMPT_CMD_READ_DUMMY_IS_VARIABLE 0xfUL
+
+#define SMPT_CMD_READ_DATA_MASK GENMASK(31, 24)
+#define SMPT_CMD_READ_DATA_SHIFT 24
+#define SMPT_CMD_READ_DATA(_cmd) \
+ ((_cmd & SMPT_CMD_READ_DATA_MASK) >> SMPT_CMD_READ_DATA_SHIFT)
+
+#define SMPT_CMD_OPCODE_MASK GENMASK(15, 8)
+#define SMPT_CMD_OPCODE_SHIFT 8
+#define SMPT_CMD_OPCODE(_cmd) \
+ ((_cmd & SMPT_CMD_OPCODE_MASK) >> SMPT_CMD_OPCODE_SHIFT)
+
+#define SMPT_MAP_REGION_COUNT_MASK GENMASK(23, 16)
+#define SMPT_MAP_REGION_COUNT_SHIFT 16
+#define SMPT_MAP_REGION_COUNT(_header) \
+ (((_header & SMPT_MAP_REGION_COUNT_MASK) >> \
+ SMPT_MAP_REGION_COUNT_SHIFT) + 1)
+
+#define SMPT_MAP_ID_MASK GENMASK(15, 8)
+#define SMPT_MAP_ID_SHIFT 8
+#define SMPT_MAP_ID(_header) ((_header & SMPT_MAP_ID_MASK) >> SMPT_MAP_ID_SHIFT)
+
+#define SMPT_MAP_REGION_SIZE_MASK GENMASK(31, 8)
+#define SMPT_MAP_REGION_SIZE_SHIFT 8
+#define SMPT_MAP_REGION_SIZE(_region) \
+ ((((_region & SMPT_MAP_REGION_SIZE_MASK) >> \
+ SMPT_MAP_REGION_SIZE_SHIFT) + 1) * 256)
+
+#define SMPT_MAP_REGION_ERASE_TYPE_MASK GENMASK(3, 0)
+#define SMPT_MAP_REGION_ERASE_TYPE(_region) \
+ (_region & SMPT_MAP_REGION_ERASE_TYPE_MASK)
+
+#define SMPT_DESC_TYPE_MAP BIT(1)
+#define SMPT_DESC_END BIT(0)
+
+static u8 spi_nor_smpt_addr_width(const struct spi_nor *nor, const u32 settings)
+{
+ switch (settings & SMPT_CMD_ADDRESS_LEN_MASK) {
+ case SMPT_CMD_ADDRESS_LEN_0:
+ return 0;
+ case SMPT_CMD_ADDRESS_LEN_3:
+ return 3;
+ case SMPT_CMD_ADDRESS_LEN_4:
+ return 4;
+ case SMPT_CMD_ADDRESS_LEN_USE_CURRENT:
+ /* fall through */
+ default:
+ return nor->addr_width;
+ }
+}
+
+static u8 spi_nor_smpt_read_dummy(const struct spi_nor *nor, const u32 settings)
+{
+ u8 read_dummy = SMPT_CMD_READ_DUMMY(settings);
+
+ if (read_dummy == SMPT_CMD_READ_DUMMY_IS_VARIABLE)
+ return nor->read_dummy;
+ return read_dummy;
+}
+
+static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt)
+{
+ const u32 *ret = NULL;
+ u32 i, addr;
+ int err;
+ u8 addr_width, read_opcode, read_dummy;
+ u8 read_data_mask, data_byte, map_id;
+
+ addr_width = nor->addr_width;
+ read_dummy = nor->read_dummy;
+ read_opcode = nor->read_opcode;
+
+ map_id = 0;
+ i = 0;
+ /* Determine if there are any optional Detection Command Descriptors */
+ while (!(smpt[i] & SMPT_DESC_TYPE_MAP)) {
+ read_data_mask = SMPT_CMD_READ_DATA(smpt[i]);
+ nor->addr_width = spi_nor_smpt_addr_width(nor, smpt[i]);
+ nor->read_dummy = spi_nor_smpt_read_dummy(nor, smpt[i]);
+ nor->read_opcode = SMPT_CMD_OPCODE(smpt[i]);
+ addr = smpt[i + 1];
+
+ err = spi_nor_read_raw(nor, addr, 1, &data_byte);
+ if (err)
+ goto out;
+
+ /*
+ * Build an index value that is used to select the Sector Map
+ * Configuration that is currently in use.
+ */
+ map_id = map_id << 1 | (!(data_byte & read_data_mask) ? 0 : 1);
+ i = i + 2;
+ }
+
+ /* Find the matching configuration map */
+ while (SMPT_MAP_ID(smpt[i]) != map_id) {
+ if (smpt[i] & SMPT_DESC_END)
+ goto out;
+ /* increment the table index to the next map */
+ i += SMPT_MAP_REGION_COUNT(smpt[i]) + 1;
+ }
+
+ ret = smpt + i;
+ /* fall through */
+out:
+ nor->addr_width = addr_width;
+ nor->read_dummy = read_dummy;
+ nor->read_opcode = read_opcode;
+ return ret;
+}
+
+static void
+spi_nor_region_check_overlay(struct spi_nor_erase_region *region,
+ const struct spi_nor_erase_type *erase,
+ const u8 erase_type)
+{
+ int i;
+
+ for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++) {
+ if (!(erase_type & BIT(i)))
+ continue;
+ if (region->size & erase[i].size_mask) {
+ spi_nor_region_mark_overlay(region);
+ return;
+ }
+ }
+}
+
+static int spi_nor_init_non_uniform_erase_map(struct spi_nor *nor,
+ const u32 *smpt)
+{
+ struct spi_nor_erase_map *map = &nor->erase_map;
+ const struct spi_nor_erase_type *erase = map->erase_type;
+ struct spi_nor_erase_region *region;
+ u64 offset;
+ u32 region_count;
+ int i, j;
+ u8 erase_type;
+
+ region_count = SMPT_MAP_REGION_COUNT(*smpt);
+ region = devm_kcalloc(nor->dev, region_count, sizeof(*region),
+ GFP_KERNEL);
+ if (!region)
+ return -ENOMEM;
+ map->regions = region;
+
+ map->uniform_erase_type = 0xff;
+ offset = 0;
+ for (i = 0; i < region_count; i++) {
+ j = i + 1; /* index for the region dword */
+ region[i].size = SMPT_MAP_REGION_SIZE(smpt[j]);
+ erase_type = SMPT_MAP_REGION_ERASE_TYPE(smpt[j]);
+ region[i].offset = offset | erase_type;
+
+ spi_nor_region_check_overlay(®ion[i], erase, erase_type);
+
+ /*
+ * Save the erase types that are supported in all regions and
+ * can erase the entire flash memory.
+ */
+ map->uniform_erase_type &= erase_type;
+
+ offset = (region[i].offset & ~SNOR_ERASE_FLAGS_MASK) +
+ region[i].size;
+ }
+
+ spi_nor_region_mark_end(®ion[i - 1]);
+
+ return 0;
+}
+
+static int spi_nor_parse_smpt(struct spi_nor *nor,
+ const struct sfdp_parameter_header *smpt_header)
+{
+ const u32 *sector_map;
+ u32 *smpt;
+ size_t len;
+ u32 addr;
+ int i, ret;
+
+ /* Read the Sector Map Parameter Table. */
+ len = smpt_header->length * sizeof(*smpt);
+ smpt = kzalloc(len, GFP_KERNEL);
+ if (!smpt)
+ return -ENOMEM;
+
+ addr = SFDP_PARAM_HEADER_PTP(smpt_header);
+ ret = spi_nor_read_sfdp(nor, addr, len, smpt);
+ if (ret)
+ goto out;
+
+ /* Fix endianness of the SMPT DWORDs. */
+ for (i = 0; i < smpt_header->length; i++)
+ smpt[i] = le32_to_cpu(smpt[i]);
+
+ sector_map = spi_nor_get_map_in_use(nor, smpt);
+ if (!sector_map) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ ret = spi_nor_init_non_uniform_erase_map(nor, sector_map);
+ if (ret)
+ goto out;
+
+ spi_nor_regions_sort_erase_types(&nor->erase_map);
+ /* fall through */
+out:
+ kfree(smpt);
+ return ret;
+}
+
/**
* spi_nor_parse_sfdp() - parse the Serial Flash Discoverable Parameters.
* @nor: pointer to a 'struct spi_nor'
@@ -2740,7 +2977,7 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
switch (SFDP_PARAM_HEADER_ID(param_header)) {
case SFDP_SECTOR_MAP_ID:
- dev_info(dev, "non-uniform erase sector maps are not supported yet.\n");
+ err = spi_nor_parse_smpt(nor, param_header);
break;
default:
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 5be9d20..5b2b0c8 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -421,6 +421,17 @@ 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 void
+spi_nor_region_mark_overlay(struct spi_nor_erase_region *region)
+{
+ region->offset |= SNOR_OVERLAID_REGION;
+}
+
static inline bool spi_nor_has_uniform_erase(const struct spi_nor *nor)
{
return !!nor->erase_map.uniform_erase_type;
--
2.9.4
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [RESEND PATCH v2 2/3] mtd: spi-nor: parse SFDP Sector Map Parameter Table
@ 2018-08-27 10:26 ` Tudor Ambarus
0 siblings, 0 replies; 22+ messages in thread
From: Tudor Ambarus @ 2018-08-27 10:26 UTC (permalink / raw)
To: linux-arm-kernel
Add support for the SFDP (JESD216B) Sector Map Parameter Table. This
table is optional, but when available, we parse it to identify the
location and size of sectors within the main data array of the
flash memory device and to identify which Erase Types are supported by
each sector.
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
drivers/mtd/spi-nor/spi-nor.c | 269 +++++++++++++++++++++++++++++++++++++++---
include/linux/mtd/spi-nor.h | 11 ++
2 files changed, 264 insertions(+), 16 deletions(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index c1e8169..522d5aa 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2085,6 +2085,35 @@ spi_nor_set_pp_settings(struct spi_nor_pp_command *pp,
*/
/**
+ * spi_nor_read_raw() - raw read of serial flash memory. read_opcode,
+ * addr_width and read_dummy members of the struct spi_nor should be previously
+ * set.
+ * @nor: pointer to a 'struct spi_nor'
+ * @addr: offset in the serial flash memory
+ * @len: number of bytes to read
+ * @buf: buffer where the data is copied into
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spi_nor_read_raw(struct spi_nor *nor, u32 addr, size_t len, u8 *buf)
+{
+ int ret;
+
+ while (len) {
+ ret = nor->read(nor, addr, len, buf);
+ if (!ret || ret > len)
+ return -EIO;
+ if (ret < 0)
+ return ret;
+
+ buf += ret;
+ addr += ret;
+ len -= ret;
+ }
+ return 0;
+}
+
+/**
* spi_nor_read_sfdp() - read Serial Flash Discoverable Parameters.
* @nor: pointer to a 'struct spi_nor'
* @addr: offset in the SFDP area to start reading data from
@@ -2111,22 +2140,8 @@ static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
nor->addr_width = 3;
nor->read_dummy = 8;
- while (len) {
- ret = nor->read(nor, addr, len, (u8 *)buf);
- if (!ret || ret > len) {
- ret = -EIO;
- goto read_err;
- }
- if (ret < 0)
- goto read_err;
+ ret = spi_nor_read_raw(nor, addr, len, buf);
- buf += ret;
- addr += ret;
- len -= ret;
- }
- ret = 0;
-
-read_err:
nor->read_opcode = read_opcode;
nor->addr_width = addr_width;
nor->read_dummy = read_dummy;
@@ -2646,6 +2661,228 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
return 0;
}
+#define SMPT_CMD_ADDRESS_LEN_MASK GENMASK(23, 22)
+#define SMPT_CMD_ADDRESS_LEN_0 (0x0UL << 22)
+#define SMPT_CMD_ADDRESS_LEN_3 (0x1UL << 22)
+#define SMPT_CMD_ADDRESS_LEN_4 (0x2UL << 22)
+#define SMPT_CMD_ADDRESS_LEN_USE_CURRENT (0x3UL << 22)
+
+#define SMPT_CMD_READ_DUMMY_MASK GENMASK(19, 16)
+#define SMPT_CMD_READ_DUMMY_SHIFT 16
+#define SMPT_CMD_READ_DUMMY(_cmd) \
+ ((_cmd & SMPT_CMD_READ_DUMMY_MASK) >> SMPT_CMD_READ_DUMMY_SHIFT)
+#define SMPT_CMD_READ_DUMMY_IS_VARIABLE 0xfUL
+
+#define SMPT_CMD_READ_DATA_MASK GENMASK(31, 24)
+#define SMPT_CMD_READ_DATA_SHIFT 24
+#define SMPT_CMD_READ_DATA(_cmd) \
+ ((_cmd & SMPT_CMD_READ_DATA_MASK) >> SMPT_CMD_READ_DATA_SHIFT)
+
+#define SMPT_CMD_OPCODE_MASK GENMASK(15, 8)
+#define SMPT_CMD_OPCODE_SHIFT 8
+#define SMPT_CMD_OPCODE(_cmd) \
+ ((_cmd & SMPT_CMD_OPCODE_MASK) >> SMPT_CMD_OPCODE_SHIFT)
+
+#define SMPT_MAP_REGION_COUNT_MASK GENMASK(23, 16)
+#define SMPT_MAP_REGION_COUNT_SHIFT 16
+#define SMPT_MAP_REGION_COUNT(_header) \
+ (((_header & SMPT_MAP_REGION_COUNT_MASK) >> \
+ SMPT_MAP_REGION_COUNT_SHIFT) + 1)
+
+#define SMPT_MAP_ID_MASK GENMASK(15, 8)
+#define SMPT_MAP_ID_SHIFT 8
+#define SMPT_MAP_ID(_header) ((_header & SMPT_MAP_ID_MASK) >> SMPT_MAP_ID_SHIFT)
+
+#define SMPT_MAP_REGION_SIZE_MASK GENMASK(31, 8)
+#define SMPT_MAP_REGION_SIZE_SHIFT 8
+#define SMPT_MAP_REGION_SIZE(_region) \
+ ((((_region & SMPT_MAP_REGION_SIZE_MASK) >> \
+ SMPT_MAP_REGION_SIZE_SHIFT) + 1) * 256)
+
+#define SMPT_MAP_REGION_ERASE_TYPE_MASK GENMASK(3, 0)
+#define SMPT_MAP_REGION_ERASE_TYPE(_region) \
+ (_region & SMPT_MAP_REGION_ERASE_TYPE_MASK)
+
+#define SMPT_DESC_TYPE_MAP BIT(1)
+#define SMPT_DESC_END BIT(0)
+
+static u8 spi_nor_smpt_addr_width(const struct spi_nor *nor, const u32 settings)
+{
+ switch (settings & SMPT_CMD_ADDRESS_LEN_MASK) {
+ case SMPT_CMD_ADDRESS_LEN_0:
+ return 0;
+ case SMPT_CMD_ADDRESS_LEN_3:
+ return 3;
+ case SMPT_CMD_ADDRESS_LEN_4:
+ return 4;
+ case SMPT_CMD_ADDRESS_LEN_USE_CURRENT:
+ /* fall through */
+ default:
+ return nor->addr_width;
+ }
+}
+
+static u8 spi_nor_smpt_read_dummy(const struct spi_nor *nor, const u32 settings)
+{
+ u8 read_dummy = SMPT_CMD_READ_DUMMY(settings);
+
+ if (read_dummy == SMPT_CMD_READ_DUMMY_IS_VARIABLE)
+ return nor->read_dummy;
+ return read_dummy;
+}
+
+static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt)
+{
+ const u32 *ret = NULL;
+ u32 i, addr;
+ int err;
+ u8 addr_width, read_opcode, read_dummy;
+ u8 read_data_mask, data_byte, map_id;
+
+ addr_width = nor->addr_width;
+ read_dummy = nor->read_dummy;
+ read_opcode = nor->read_opcode;
+
+ map_id = 0;
+ i = 0;
+ /* Determine if there are any optional Detection Command Descriptors */
+ while (!(smpt[i] & SMPT_DESC_TYPE_MAP)) {
+ read_data_mask = SMPT_CMD_READ_DATA(smpt[i]);
+ nor->addr_width = spi_nor_smpt_addr_width(nor, smpt[i]);
+ nor->read_dummy = spi_nor_smpt_read_dummy(nor, smpt[i]);
+ nor->read_opcode = SMPT_CMD_OPCODE(smpt[i]);
+ addr = smpt[i + 1];
+
+ err = spi_nor_read_raw(nor, addr, 1, &data_byte);
+ if (err)
+ goto out;
+
+ /*
+ * Build an index value that is used to select the Sector Map
+ * Configuration that is currently in use.
+ */
+ map_id = map_id << 1 | (!(data_byte & read_data_mask) ? 0 : 1);
+ i = i + 2;
+ }
+
+ /* Find the matching configuration map */
+ while (SMPT_MAP_ID(smpt[i]) != map_id) {
+ if (smpt[i] & SMPT_DESC_END)
+ goto out;
+ /* increment the table index to the next map */
+ i += SMPT_MAP_REGION_COUNT(smpt[i]) + 1;
+ }
+
+ ret = smpt + i;
+ /* fall through */
+out:
+ nor->addr_width = addr_width;
+ nor->read_dummy = read_dummy;
+ nor->read_opcode = read_opcode;
+ return ret;
+}
+
+static void
+spi_nor_region_check_overlay(struct spi_nor_erase_region *region,
+ const struct spi_nor_erase_type *erase,
+ const u8 erase_type)
+{
+ int i;
+
+ for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++) {
+ if (!(erase_type & BIT(i)))
+ continue;
+ if (region->size & erase[i].size_mask) {
+ spi_nor_region_mark_overlay(region);
+ return;
+ }
+ }
+}
+
+static int spi_nor_init_non_uniform_erase_map(struct spi_nor *nor,
+ const u32 *smpt)
+{
+ struct spi_nor_erase_map *map = &nor->erase_map;
+ const struct spi_nor_erase_type *erase = map->erase_type;
+ struct spi_nor_erase_region *region;
+ u64 offset;
+ u32 region_count;
+ int i, j;
+ u8 erase_type;
+
+ region_count = SMPT_MAP_REGION_COUNT(*smpt);
+ region = devm_kcalloc(nor->dev, region_count, sizeof(*region),
+ GFP_KERNEL);
+ if (!region)
+ return -ENOMEM;
+ map->regions = region;
+
+ map->uniform_erase_type = 0xff;
+ offset = 0;
+ for (i = 0; i < region_count; i++) {
+ j = i + 1; /* index for the region dword */
+ region[i].size = SMPT_MAP_REGION_SIZE(smpt[j]);
+ erase_type = SMPT_MAP_REGION_ERASE_TYPE(smpt[j]);
+ region[i].offset = offset | erase_type;
+
+ spi_nor_region_check_overlay(®ion[i], erase, erase_type);
+
+ /*
+ * Save the erase types that are supported in all regions and
+ * can erase the entire flash memory.
+ */
+ map->uniform_erase_type &= erase_type;
+
+ offset = (region[i].offset & ~SNOR_ERASE_FLAGS_MASK) +
+ region[i].size;
+ }
+
+ spi_nor_region_mark_end(®ion[i - 1]);
+
+ return 0;
+}
+
+static int spi_nor_parse_smpt(struct spi_nor *nor,
+ const struct sfdp_parameter_header *smpt_header)
+{
+ const u32 *sector_map;
+ u32 *smpt;
+ size_t len;
+ u32 addr;
+ int i, ret;
+
+ /* Read the Sector Map Parameter Table. */
+ len = smpt_header->length * sizeof(*smpt);
+ smpt = kzalloc(len, GFP_KERNEL);
+ if (!smpt)
+ return -ENOMEM;
+
+ addr = SFDP_PARAM_HEADER_PTP(smpt_header);
+ ret = spi_nor_read_sfdp(nor, addr, len, smpt);
+ if (ret)
+ goto out;
+
+ /* Fix endianness of the SMPT DWORDs. */
+ for (i = 0; i < smpt_header->length; i++)
+ smpt[i] = le32_to_cpu(smpt[i]);
+
+ sector_map = spi_nor_get_map_in_use(nor, smpt);
+ if (!sector_map) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ ret = spi_nor_init_non_uniform_erase_map(nor, sector_map);
+ if (ret)
+ goto out;
+
+ spi_nor_regions_sort_erase_types(&nor->erase_map);
+ /* fall through */
+out:
+ kfree(smpt);
+ return ret;
+}
+
/**
* spi_nor_parse_sfdp() - parse the Serial Flash Discoverable Parameters.
* @nor: pointer to a 'struct spi_nor'
@@ -2740,7 +2977,7 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
switch (SFDP_PARAM_HEADER_ID(param_header)) {
case SFDP_SECTOR_MAP_ID:
- dev_info(dev, "non-uniform erase sector maps are not supported yet.\n");
+ err = spi_nor_parse_smpt(nor, param_header);
break;
default:
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 5be9d20..5b2b0c8 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -421,6 +421,17 @@ 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 void
+spi_nor_region_mark_overlay(struct spi_nor_erase_region *region)
+{
+ region->offset |= SNOR_OVERLAID_REGION;
+}
+
static inline bool spi_nor_has_uniform_erase(const struct spi_nor *nor)
{
return !!nor->erase_map.uniform_erase_type;
--
2.9.4
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [RESEND PATCH v2 3/3] mtd: spi-nor: parse SFDP 4-byte Address Instruction Table
2018-08-27 10:26 ` Tudor Ambarus
@ 2018-08-27 10:26 ` Tudor Ambarus
-1 siblings, 0 replies; 22+ messages in thread
From: Tudor Ambarus @ 2018-08-27 10:26 UTC (permalink / raw)
To: marek.vasut, cyrille.pitchen, dwmw2, computersforpeace,
boris.brezillon, richard
Cc: linux-mtd, linux-arm-kernel, linux-kernel, nicolas.ferre,
Cristian.Birsan, Tudor Ambarus
From: Cyrille Pitchen <cyrille.pitchen@microchip.com>
Add support for SFDP (JESD216B) 4-byte Address Instruction Table. This
table is optional but when available, we parse it to get the 4-byte
address op codes supported by the memory.
Using these op codes is stateless as opposed to entering the 4-byte
address mode or setting the Base Address Register (BAR).
Signed-off-by: Cyrille Pitchen <cyrille.pitchen@microchip.com>
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
drivers/mtd/spi-nor/spi-nor.c | 148 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 148 insertions(+)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 522d5aa..4e69d47 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2196,6 +2196,7 @@ struct sfdp_parameter_header {
#define SFDP_BFPT_ID 0xff00 /* Basic Flash Parameter Table */
#define SFDP_SECTOR_MAP_ID 0xff81 /* Sector Map Table */
+#define SFDP_4BAIT_ID 0xff84u /* 4-byte Address Instruction Table */
#define SFDP_SIGNATURE 0x50444653U
#define SFDP_JESD216_MAJOR 1
@@ -2883,6 +2884,149 @@ static int spi_nor_parse_smpt(struct spi_nor *nor,
return ret;
}
+struct sfdp_4bait {
+ /* The hardware capability. */
+ u32 hwcaps;
+
+ /*
+ * The <supported_bit> bit in DWORD1 of the 4BAIT tells us whether
+ * the associated 4-byte address op code is supported.
+ */
+ u32 supported_bit;
+};
+
+static int spi_nor_parse_4bait(struct spi_nor *nor,
+ const struct sfdp_parameter_header *param_header,
+ struct spi_nor_flash_parameter *params)
+{
+ static const struct sfdp_4bait reads[] = {
+ { SNOR_HWCAPS_READ, BIT(0) },
+ { SNOR_HWCAPS_READ_FAST, BIT(1) },
+ { SNOR_HWCAPS_READ_1_1_2, BIT(2) },
+ { SNOR_HWCAPS_READ_1_2_2, BIT(3) },
+ { SNOR_HWCAPS_READ_1_1_4, BIT(4) },
+ { SNOR_HWCAPS_READ_1_4_4, BIT(5) },
+ { SNOR_HWCAPS_READ_1_1_1_DTR, BIT(13) },
+ { SNOR_HWCAPS_READ_1_2_2_DTR, BIT(14) },
+ { SNOR_HWCAPS_READ_1_4_4_DTR, BIT(15) },
+ };
+ static const struct sfdp_4bait programs[] = {
+ { SNOR_HWCAPS_PP, BIT(6) },
+ { SNOR_HWCAPS_PP_1_1_4, BIT(7) },
+ { SNOR_HWCAPS_PP_1_4_4, BIT(8) },
+ };
+ static const struct sfdp_4bait erases[SNOR_ERASE_TYPE_MAX] = {
+ { 0u /* not used */, BIT(9) },
+ { 0u /* not used */, BIT(10) },
+ { 0u /* not used */, BIT(11) },
+ { 0u /* not used */, BIT(12) },
+ };
+ u32 dwords[2], addr, discard_hwcaps, read_hwcaps, pp_hwcaps, erase_mask;
+ struct spi_nor_erase_map *map = &nor->erase_map;
+ int i, err;
+
+ if (param_header->major != SFDP_JESD216_MAJOR ||
+ param_header->length < ARRAY_SIZE(dwords))
+ return -EINVAL;
+
+ /* Read the 4-byte Address Instruction Table. */
+ addr = SFDP_PARAM_HEADER_PTP(param_header);
+ err = spi_nor_read_sfdp(nor, addr, sizeof(dwords), dwords);
+ if (err)
+ return err;
+
+ /* Fix endianness of the 4BAIT DWORDs. */
+ for (i = 0; i < ARRAY_SIZE(dwords); i++)
+ dwords[i] = le32_to_cpu(dwords[i]);
+
+ /*
+ * Compute the subset of (Fast) Read commands for which the 4-byte
+ * version is supported.
+ */
+ discard_hwcaps = 0;
+ read_hwcaps = 0;
+ for (i = 0; i < ARRAY_SIZE(reads); i++) {
+ const struct sfdp_4bait *read = &reads[i];
+
+ discard_hwcaps |= read->hwcaps;
+ if ((params->hwcaps.mask & read->hwcaps) &&
+ (dwords[0] & read->supported_bit))
+ read_hwcaps |= read->hwcaps;
+ }
+
+ /*
+ * Compute the subset of Page Program commands for which the 4-byte
+ * version is supported.
+ */
+ pp_hwcaps = 0;
+ for (i = 0; i < ARRAY_SIZE(programs); i++) {
+ const struct sfdp_4bait *program = &programs[i];
+
+ discard_hwcaps |= program->hwcaps;
+ if ((params->hwcaps.mask & program->hwcaps) &&
+ (dwords[0] & program->supported_bit))
+ pp_hwcaps |= program->hwcaps;
+ }
+
+ /*
+ * Compute the subet of Sector Erase commands for which the 4-byte
+ * version is supported.
+ */
+ erase_mask = 0;
+ for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++) {
+ const struct sfdp_4bait *erase = &erases[i];
+
+ if (map->erase_type[i].size > 0 &&
+ (dwords[0] & erase->supported_bit))
+ erase_mask |= BIT(i);
+ }
+
+ /*
+ * We need at least one 4-byte op code per read, program and erase
+ * operation; the .read(), .write() and .erase() hooks share the
+ * nor->addr_width value.
+ */
+ if (!read_hwcaps || !pp_hwcaps || !erase_mask)
+ return 0;
+
+ /*
+ * Discard all operations from the 4-byte instruction set which are
+ * not supported by this memory.
+ */
+ params->hwcaps.mask &= ~discard_hwcaps;
+ params->hwcaps.mask |= (read_hwcaps | pp_hwcaps);
+
+ /* Use the 4-byte address instruction set. */
+ for (i = 0; i < SNOR_CMD_READ_MAX; i++) {
+ struct spi_nor_read_command *read_cmd = ¶ms->reads[i];
+
+ read_cmd->opcode = spi_nor_convert_3to4_read(read_cmd->opcode);
+ }
+ for (i = 0; i < SNOR_CMD_PP_MAX; i++) {
+ struct spi_nor_pp_command *pp_cmd = ¶ms->page_programs[i];
+
+ pp_cmd->opcode = spi_nor_convert_3to4_program(pp_cmd->opcode);
+ }
+ for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++) {
+ struct spi_nor_erase_type *erase = &map->erase_type[i];
+
+ if (erase_mask & BIT(erase->idx))
+ erase->opcode = (dwords[1] >> (erase->idx * 8)) & 0xFF;
+ else
+ spi_nor_set_erase_type(erase, 0u, 0xFF);
+ }
+
+ /*
+ * We set nor->addr_width here to skip spi_nor_set_4byte_opcodes()
+ * later because this latest function implements a legacy quirk for
+ * the erase size of Spansion memory. However this quirk is no longer
+ * needed with new SFDP compliant memories.
+ */
+ nor->addr_width = 4;
+ nor->flags |= SPI_NOR_4B_OPCODES;
+ return 0;
+}
+
/**
* spi_nor_parse_sfdp() - parse the Serial Flash Discoverable Parameters.
* @nor: pointer to a 'struct spi_nor'
@@ -2980,6 +3124,10 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
err = spi_nor_parse_smpt(nor, param_header);
break;
+ case SFDP_4BAIT_ID:
+ err = spi_nor_parse_4bait(nor, param_header, params);
+ break;
+
default:
break;
}
--
2.9.4
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [RESEND PATCH v2 3/3] mtd: spi-nor: parse SFDP 4-byte Address Instruction Table
@ 2018-08-27 10:26 ` Tudor Ambarus
0 siblings, 0 replies; 22+ messages in thread
From: Tudor Ambarus @ 2018-08-27 10:26 UTC (permalink / raw)
To: linux-arm-kernel
From: Cyrille Pitchen <cyrille.pitchen@microchip.com>
Add support for SFDP (JESD216B) 4-byte Address Instruction Table. This
table is optional but when available, we parse it to get the 4-byte
address op codes supported by the memory.
Using these op codes is stateless as opposed to entering the 4-byte
address mode or setting the Base Address Register (BAR).
Signed-off-by: Cyrille Pitchen <cyrille.pitchen@microchip.com>
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
drivers/mtd/spi-nor/spi-nor.c | 148 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 148 insertions(+)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 522d5aa..4e69d47 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2196,6 +2196,7 @@ struct sfdp_parameter_header {
#define SFDP_BFPT_ID 0xff00 /* Basic Flash Parameter Table */
#define SFDP_SECTOR_MAP_ID 0xff81 /* Sector Map Table */
+#define SFDP_4BAIT_ID 0xff84u /* 4-byte Address Instruction Table */
#define SFDP_SIGNATURE 0x50444653U
#define SFDP_JESD216_MAJOR 1
@@ -2883,6 +2884,149 @@ static int spi_nor_parse_smpt(struct spi_nor *nor,
return ret;
}
+struct sfdp_4bait {
+ /* The hardware capability. */
+ u32 hwcaps;
+
+ /*
+ * The <supported_bit> bit in DWORD1 of the 4BAIT tells us whether
+ * the associated 4-byte address op code is supported.
+ */
+ u32 supported_bit;
+};
+
+static int spi_nor_parse_4bait(struct spi_nor *nor,
+ const struct sfdp_parameter_header *param_header,
+ struct spi_nor_flash_parameter *params)
+{
+ static const struct sfdp_4bait reads[] = {
+ { SNOR_HWCAPS_READ, BIT(0) },
+ { SNOR_HWCAPS_READ_FAST, BIT(1) },
+ { SNOR_HWCAPS_READ_1_1_2, BIT(2) },
+ { SNOR_HWCAPS_READ_1_2_2, BIT(3) },
+ { SNOR_HWCAPS_READ_1_1_4, BIT(4) },
+ { SNOR_HWCAPS_READ_1_4_4, BIT(5) },
+ { SNOR_HWCAPS_READ_1_1_1_DTR, BIT(13) },
+ { SNOR_HWCAPS_READ_1_2_2_DTR, BIT(14) },
+ { SNOR_HWCAPS_READ_1_4_4_DTR, BIT(15) },
+ };
+ static const struct sfdp_4bait programs[] = {
+ { SNOR_HWCAPS_PP, BIT(6) },
+ { SNOR_HWCAPS_PP_1_1_4, BIT(7) },
+ { SNOR_HWCAPS_PP_1_4_4, BIT(8) },
+ };
+ static const struct sfdp_4bait erases[SNOR_ERASE_TYPE_MAX] = {
+ { 0u /* not used */, BIT(9) },
+ { 0u /* not used */, BIT(10) },
+ { 0u /* not used */, BIT(11) },
+ { 0u /* not used */, BIT(12) },
+ };
+ u32 dwords[2], addr, discard_hwcaps, read_hwcaps, pp_hwcaps, erase_mask;
+ struct spi_nor_erase_map *map = &nor->erase_map;
+ int i, err;
+
+ if (param_header->major != SFDP_JESD216_MAJOR ||
+ param_header->length < ARRAY_SIZE(dwords))
+ return -EINVAL;
+
+ /* Read the 4-byte Address Instruction Table. */
+ addr = SFDP_PARAM_HEADER_PTP(param_header);
+ err = spi_nor_read_sfdp(nor, addr, sizeof(dwords), dwords);
+ if (err)
+ return err;
+
+ /* Fix endianness of the 4BAIT DWORDs. */
+ for (i = 0; i < ARRAY_SIZE(dwords); i++)
+ dwords[i] = le32_to_cpu(dwords[i]);
+
+ /*
+ * Compute the subset of (Fast) Read commands for which the 4-byte
+ * version is supported.
+ */
+ discard_hwcaps = 0;
+ read_hwcaps = 0;
+ for (i = 0; i < ARRAY_SIZE(reads); i++) {
+ const struct sfdp_4bait *read = &reads[i];
+
+ discard_hwcaps |= read->hwcaps;
+ if ((params->hwcaps.mask & read->hwcaps) &&
+ (dwords[0] & read->supported_bit))
+ read_hwcaps |= read->hwcaps;
+ }
+
+ /*
+ * Compute the subset of Page Program commands for which the 4-byte
+ * version is supported.
+ */
+ pp_hwcaps = 0;
+ for (i = 0; i < ARRAY_SIZE(programs); i++) {
+ const struct sfdp_4bait *program = &programs[i];
+
+ discard_hwcaps |= program->hwcaps;
+ if ((params->hwcaps.mask & program->hwcaps) &&
+ (dwords[0] & program->supported_bit))
+ pp_hwcaps |= program->hwcaps;
+ }
+
+ /*
+ * Compute the subet of Sector Erase commands for which the 4-byte
+ * version is supported.
+ */
+ erase_mask = 0;
+ for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++) {
+ const struct sfdp_4bait *erase = &erases[i];
+
+ if (map->erase_type[i].size > 0 &&
+ (dwords[0] & erase->supported_bit))
+ erase_mask |= BIT(i);
+ }
+
+ /*
+ * We need at least one 4-byte op code per read, program and erase
+ * operation; the .read(), .write() and .erase() hooks share the
+ * nor->addr_width value.
+ */
+ if (!read_hwcaps || !pp_hwcaps || !erase_mask)
+ return 0;
+
+ /*
+ * Discard all operations from the 4-byte instruction set which are
+ * not supported by this memory.
+ */
+ params->hwcaps.mask &= ~discard_hwcaps;
+ params->hwcaps.mask |= (read_hwcaps | pp_hwcaps);
+
+ /* Use the 4-byte address instruction set. */
+ for (i = 0; i < SNOR_CMD_READ_MAX; i++) {
+ struct spi_nor_read_command *read_cmd = ¶ms->reads[i];
+
+ read_cmd->opcode = spi_nor_convert_3to4_read(read_cmd->opcode);
+ }
+ for (i = 0; i < SNOR_CMD_PP_MAX; i++) {
+ struct spi_nor_pp_command *pp_cmd = ¶ms->page_programs[i];
+
+ pp_cmd->opcode = spi_nor_convert_3to4_program(pp_cmd->opcode);
+ }
+ for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++) {
+ struct spi_nor_erase_type *erase = &map->erase_type[i];
+
+ if (erase_mask & BIT(erase->idx))
+ erase->opcode = (dwords[1] >> (erase->idx * 8)) & 0xFF;
+ else
+ spi_nor_set_erase_type(erase, 0u, 0xFF);
+ }
+
+ /*
+ * We set nor->addr_width here to skip spi_nor_set_4byte_opcodes()
+ * later because this latest function implements a legacy quirk for
+ * the erase size of Spansion memory. However this quirk is no longer
+ * needed with new SFDP compliant memories.
+ */
+ nor->addr_width = 4;
+ nor->flags |= SPI_NOR_4B_OPCODES;
+ return 0;
+}
+
/**
* spi_nor_parse_sfdp() - parse the Serial Flash Discoverable Parameters.
* @nor: pointer to a 'struct spi_nor'
@@ -2980,6 +3124,10 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
err = spi_nor_parse_smpt(nor, param_header);
break;
+ case SFDP_4BAIT_ID:
+ err = spi_nor_parse_4bait(nor, param_header, params);
+ break;
+
default:
break;
}
--
2.9.4
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [RESEND PATCH v2 1/3] mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories
2018-08-27 10:26 ` Tudor Ambarus
@ 2018-09-03 17:37 ` Marek Vasut
-1 siblings, 0 replies; 22+ messages in thread
From: Marek Vasut @ 2018-09-03 17:37 UTC (permalink / raw)
To: Tudor Ambarus, cyrille.pitchen, dwmw2, computersforpeace,
boris.brezillon, richard
Cc: linux-mtd, linux-arm-kernel, linux-kernel, nicolas.ferre,
Cristian.Birsan
On 08/27/2018 12:26 PM, Tudor Ambarus wrote:
[...]
> +/* 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_type *erase,
> + u64 dividend, u32 *remainder)
> +{
> + *remainder = (u32)dividend & erase->size_mask;
Is the cast really needed ? btw I think there might be a macro doing
just this, div_by_ or something in include/ .
> + return dividend >> erase->size_shift;
> +}
> +
> +static const struct spi_nor_erase_type *
> +spi_nor_find_best_erase_type(const struct spi_nor_erase_map *map,
> + const struct spi_nor_erase_region *region,
> + u64 addr, u32 len)
> +{
> + const struct spi_nor_erase_type *erase;
> + u32 rem;
> + int i;
> + u8 erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
> +
> + /*
> + * Erase types are ordered by size, with the biggest erase type at
> + * index 0.
> + */
> + for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
> + /* Does the erase region support the tested erase type? */
> + if (!(erase_mask & BIT(i)))
> + continue;
> +
> + erase = &map->erase_type[i];
> +
> + /* Don't erase more than what the user has asked for. */
> + if (erase->size > len)
> + continue;
> +
> + /* Alignment is not mandatory for overlaid regions */
> + if (region->offset & SNOR_OVERLAID_REGION)
> + return erase;
> +
> + spi_nor_div_by_erase_size(erase, addr, &rem);
> + if (rem)
> + continue;
> + else
> + return erase;
> + }
> +
> + return NULL;
> +}
> +
> +static struct spi_nor_erase_region *
> +spi_nor_region_next(struct spi_nor_erase_region *region)
> +{
> + if (spi_nor_region_is_last(region))
> + return NULL;
> + return ++region;
region++ ...
[...]
> +static int spi_nor_cmp_erase_type(const void *a, const void *b)
> +{
> + const struct spi_nor_erase_type *erase1 = a;
> + const struct spi_nor_erase_type *erase2 = b;
> +
> + return erase1->size - erase2->size;
What does this function do again ?
> +}
> +
> +static void spi_nor_regions_sort_erase_types(struct spi_nor_erase_map *map)
> +{
> + struct spi_nor_erase_region *region = map->regions;
> + struct spi_nor_erase_type *erase_type = map->erase_type;
> + int i;
> + u8 region_erase_mask, ordered_erase_mask;
> +
> + /*
> + * Sort each region's Erase Types in ascending order with the smallest
> + * Erase Type size starting at BIT(0).
> + */
> + while (region) {
> + region_erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
> +
> + /*
> + * The region's erase mask indicates which erase types are
> + * supported from the erase types defined in the map.
> + */
> + ordered_erase_mask = 0;
> + for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++)
> + if (erase_type[i].size &&
> + region_erase_mask & BIT(erase_type[i].idx))
> + ordered_erase_mask |= BIT(i);
> +
> + /* Overwrite erase mask. */
> + region->offset = (region->offset & ~SNOR_ERASE_TYPE_MASK) |
> + ordered_erase_mask;
> +
> + region = spi_nor_region_next(region);
> + }
> +}
> +
> +static inline void
Drop the inline
> +spi_nor_init_uniform_erase_map(struct spi_nor_erase_map *map,
> + u8 erase_mask, u64 flash_size)
> +{
> + map->uniform_region.offset = SNOR_ERASE_FLAGS_OFFSET(erase_mask, 1, 0,
> + 0);
> + map->uniform_region.size = flash_size;
> + map->regions = &map->uniform_region;
> + map->uniform_erase_type = erase_mask;
> +}
> +
[...]
> +#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)
Get rid of the inlines, really.
> +{
> + return (region->offset & ~SNOR_ERASE_FLAGS_MASK) + region->size;
> +}
> +
> +static inline bool spi_nor_has_uniform_erase(const struct spi_nor *nor)
> +{
> + return !!nor->erase_map.uniform_erase_type;
> +}
> +
> static inline void spi_nor_set_flash_node(struct spi_nor *nor,
> struct device_node *np)
> {
>
General question, what happens if the multi-block erase fails mid-way ,
is that handled or reported somehow to the user ?
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RESEND PATCH v2 1/3] mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories
@ 2018-09-03 17:37 ` Marek Vasut
0 siblings, 0 replies; 22+ messages in thread
From: Marek Vasut @ 2018-09-03 17:37 UTC (permalink / raw)
To: linux-arm-kernel
On 08/27/2018 12:26 PM, Tudor Ambarus wrote:
[...]
> +/* 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_type *erase,
> + u64 dividend, u32 *remainder)
> +{
> + *remainder = (u32)dividend & erase->size_mask;
Is the cast really needed ? btw I think there might be a macro doing
just this, div_by_ or something in include/ .
> + return dividend >> erase->size_shift;
> +}
> +
> +static const struct spi_nor_erase_type *
> +spi_nor_find_best_erase_type(const struct spi_nor_erase_map *map,
> + const struct spi_nor_erase_region *region,
> + u64 addr, u32 len)
> +{
> + const struct spi_nor_erase_type *erase;
> + u32 rem;
> + int i;
> + u8 erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
> +
> + /*
> + * Erase types are ordered by size, with the biggest erase type at
> + * index 0.
> + */
> + for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
> + /* Does the erase region support the tested erase type? */
> + if (!(erase_mask & BIT(i)))
> + continue;
> +
> + erase = &map->erase_type[i];
> +
> + /* Don't erase more than what the user has asked for. */
> + if (erase->size > len)
> + continue;
> +
> + /* Alignment is not mandatory for overlaid regions */
> + if (region->offset & SNOR_OVERLAID_REGION)
> + return erase;
> +
> + spi_nor_div_by_erase_size(erase, addr, &rem);
> + if (rem)
> + continue;
> + else
> + return erase;
> + }
> +
> + return NULL;
> +}
> +
> +static struct spi_nor_erase_region *
> +spi_nor_region_next(struct spi_nor_erase_region *region)
> +{
> + if (spi_nor_region_is_last(region))
> + return NULL;
> + return ++region;
region++ ...
[...]
> +static int spi_nor_cmp_erase_type(const void *a, const void *b)
> +{
> + const struct spi_nor_erase_type *erase1 = a;
> + const struct spi_nor_erase_type *erase2 = b;
> +
> + return erase1->size - erase2->size;
What does this function do again ?
> +}
> +
> +static void spi_nor_regions_sort_erase_types(struct spi_nor_erase_map *map)
> +{
> + struct spi_nor_erase_region *region = map->regions;
> + struct spi_nor_erase_type *erase_type = map->erase_type;
> + int i;
> + u8 region_erase_mask, ordered_erase_mask;
> +
> + /*
> + * Sort each region's Erase Types in ascending order with the smallest
> + * Erase Type size starting at BIT(0).
> + */
> + while (region) {
> + region_erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
> +
> + /*
> + * The region's erase mask indicates which erase types are
> + * supported from the erase types defined in the map.
> + */
> + ordered_erase_mask = 0;
> + for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++)
> + if (erase_type[i].size &&
> + region_erase_mask & BIT(erase_type[i].idx))
> + ordered_erase_mask |= BIT(i);
> +
> + /* Overwrite erase mask. */
> + region->offset = (region->offset & ~SNOR_ERASE_TYPE_MASK) |
> + ordered_erase_mask;
> +
> + region = spi_nor_region_next(region);
> + }
> +}
> +
> +static inline void
Drop the inline
> +spi_nor_init_uniform_erase_map(struct spi_nor_erase_map *map,
> + u8 erase_mask, u64 flash_size)
> +{
> + map->uniform_region.offset = SNOR_ERASE_FLAGS_OFFSET(erase_mask, 1, 0,
> + 0);
> + map->uniform_region.size = flash_size;
> + map->regions = &map->uniform_region;
> + map->uniform_erase_type = erase_mask;
> +}
> +
[...]
> +#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)
Get rid of the inlines, really.
> +{
> + return (region->offset & ~SNOR_ERASE_FLAGS_MASK) + region->size;
> +}
> +
> +static inline bool spi_nor_has_uniform_erase(const struct spi_nor *nor)
> +{
> + return !!nor->erase_map.uniform_erase_type;
> +}
> +
> static inline void spi_nor_set_flash_node(struct spi_nor *nor,
> struct device_node *np)
> {
>
General question, what happens if the multi-block erase fails mid-way ,
is that handled or reported somehow to the user ?
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RESEND PATCH v2 2/3] mtd: spi-nor: parse SFDP Sector Map Parameter Table
2018-08-27 10:26 ` Tudor Ambarus
@ 2018-09-03 17:40 ` Marek Vasut
-1 siblings, 0 replies; 22+ messages in thread
From: Marek Vasut @ 2018-09-03 17:40 UTC (permalink / raw)
To: Tudor Ambarus, cyrille.pitchen, dwmw2, computersforpeace,
boris.brezillon, richard
Cc: linux-mtd, linux-arm-kernel, linux-kernel, nicolas.ferre,
Cristian.Birsan
On 08/27/2018 12:26 PM, Tudor Ambarus wrote:
[...]
> +static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt)
> +{
> + const u32 *ret = NULL;
> + u32 i, addr;
> + int err;
> + u8 addr_width, read_opcode, read_dummy;
> + u8 read_data_mask, data_byte, map_id;
> +
> + addr_width = nor->addr_width;
> + read_dummy = nor->read_dummy;
> + read_opcode = nor->read_opcode;
> +
> + map_id = 0;
> + i = 0;
> + /* Determine if there are any optional Detection Command Descriptors */
> + while (!(smpt[i] & SMPT_DESC_TYPE_MAP)) {
> + read_data_mask = SMPT_CMD_READ_DATA(smpt[i]);
> + nor->addr_width = spi_nor_smpt_addr_width(nor, smpt[i]);
> + nor->read_dummy = spi_nor_smpt_read_dummy(nor, smpt[i]);
> + nor->read_opcode = SMPT_CMD_OPCODE(smpt[i]);
> + addr = smpt[i + 1];
> +
> + err = spi_nor_read_raw(nor, addr, 1, &data_byte);
> + if (err)
> + goto out;
> +
> + /*
> + * Build an index value that is used to select the Sector Map
> + * Configuration that is currently in use.
> + */
> + map_id = map_id << 1 | (!(data_byte & read_data_mask) ? 0 : 1);
You can drop the ternary operator part completely ^
> + i = i + 2;
> + }
> +
> + /* Find the matching configuration map */
> + while (SMPT_MAP_ID(smpt[i]) != map_id) {
> + if (smpt[i] & SMPT_DESC_END)
> + goto out;
> + /* increment the table index to the next map */
> + i += SMPT_MAP_REGION_COUNT(smpt[i]) + 1;
> + }
> +
> + ret = smpt + i;
> + /* fall through */
> +out:
> + nor->addr_width = addr_width;
> + nor->read_dummy = read_dummy;
> + nor->read_opcode = read_opcode;
> + return ret;
> +}
> +
> +static void
> +spi_nor_region_check_overlay(struct spi_nor_erase_region *region,
> + const struct spi_nor_erase_type *erase,
> + const u8 erase_type)
> +{
> + int i;
> +
> + for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++) {
> + if (!(erase_type & BIT(i)))
> + continue;
> + if (region->size & erase[i].size_mask) {
> + spi_nor_region_mark_overlay(region);
> + return;
> + }
> + }
> +}
> +
> +static int spi_nor_init_non_uniform_erase_map(struct spi_nor *nor,
> + const u32 *smpt)
> +{
> + struct spi_nor_erase_map *map = &nor->erase_map;
> + const struct spi_nor_erase_type *erase = map->erase_type;
> + struct spi_nor_erase_region *region;
> + u64 offset;
> + u32 region_count;
> + int i, j;
> + u8 erase_type;
> +
> + region_count = SMPT_MAP_REGION_COUNT(*smpt);
> + region = devm_kcalloc(nor->dev, region_count, sizeof(*region),
> + GFP_KERNEL);
Is this memory always correctly free'd ?
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RESEND PATCH v2 2/3] mtd: spi-nor: parse SFDP Sector Map Parameter Table
@ 2018-09-03 17:40 ` Marek Vasut
0 siblings, 0 replies; 22+ messages in thread
From: Marek Vasut @ 2018-09-03 17:40 UTC (permalink / raw)
To: linux-arm-kernel
On 08/27/2018 12:26 PM, Tudor Ambarus wrote:
[...]
> +static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt)
> +{
> + const u32 *ret = NULL;
> + u32 i, addr;
> + int err;
> + u8 addr_width, read_opcode, read_dummy;
> + u8 read_data_mask, data_byte, map_id;
> +
> + addr_width = nor->addr_width;
> + read_dummy = nor->read_dummy;
> + read_opcode = nor->read_opcode;
> +
> + map_id = 0;
> + i = 0;
> + /* Determine if there are any optional Detection Command Descriptors */
> + while (!(smpt[i] & SMPT_DESC_TYPE_MAP)) {
> + read_data_mask = SMPT_CMD_READ_DATA(smpt[i]);
> + nor->addr_width = spi_nor_smpt_addr_width(nor, smpt[i]);
> + nor->read_dummy = spi_nor_smpt_read_dummy(nor, smpt[i]);
> + nor->read_opcode = SMPT_CMD_OPCODE(smpt[i]);
> + addr = smpt[i + 1];
> +
> + err = spi_nor_read_raw(nor, addr, 1, &data_byte);
> + if (err)
> + goto out;
> +
> + /*
> + * Build an index value that is used to select the Sector Map
> + * Configuration that is currently in use.
> + */
> + map_id = map_id << 1 | (!(data_byte & read_data_mask) ? 0 : 1);
You can drop the ternary operator part completely ^
> + i = i + 2;
> + }
> +
> + /* Find the matching configuration map */
> + while (SMPT_MAP_ID(smpt[i]) != map_id) {
> + if (smpt[i] & SMPT_DESC_END)
> + goto out;
> + /* increment the table index to the next map */
> + i += SMPT_MAP_REGION_COUNT(smpt[i]) + 1;
> + }
> +
> + ret = smpt + i;
> + /* fall through */
> +out:
> + nor->addr_width = addr_width;
> + nor->read_dummy = read_dummy;
> + nor->read_opcode = read_opcode;
> + return ret;
> +}
> +
> +static void
> +spi_nor_region_check_overlay(struct spi_nor_erase_region *region,
> + const struct spi_nor_erase_type *erase,
> + const u8 erase_type)
> +{
> + int i;
> +
> + for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++) {
> + if (!(erase_type & BIT(i)))
> + continue;
> + if (region->size & erase[i].size_mask) {
> + spi_nor_region_mark_overlay(region);
> + return;
> + }
> + }
> +}
> +
> +static int spi_nor_init_non_uniform_erase_map(struct spi_nor *nor,
> + const u32 *smpt)
> +{
> + struct spi_nor_erase_map *map = &nor->erase_map;
> + const struct spi_nor_erase_type *erase = map->erase_type;
> + struct spi_nor_erase_region *region;
> + u64 offset;
> + u32 region_count;
> + int i, j;
> + u8 erase_type;
> +
> + region_count = SMPT_MAP_REGION_COUNT(*smpt);
> + region = devm_kcalloc(nor->dev, region_count, sizeof(*region),
> + GFP_KERNEL);
Is this memory always correctly free'd ?
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RESEND PATCH v2 1/3] mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories
2018-09-03 17:37 ` Marek Vasut
@ 2018-09-07 8:51 ` Tudor Ambarus
-1 siblings, 0 replies; 22+ messages in thread
From: Tudor Ambarus @ 2018-09-07 8:51 UTC (permalink / raw)
To: Marek Vasut, cyrille.pitchen, dwmw2, computersforpeace,
boris.brezillon, richard
Cc: linux-mtd, linux-arm-kernel, linux-kernel, nicolas.ferre,
Cristian.Birsan
Thanks Marek,
On 09/03/2018 08:37 PM, Marek Vasut wrote:
> On 08/27/2018 12:26 PM, Tudor Ambarus wrote:
> [...]
>
>> +/* 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_type *erase,
>> + u64 dividend, u32 *remainder)
>> +{
>> + *remainder = (u32)dividend & erase->size_mask;
>
> Is the cast really needed ? btw I think there might be a macro doing
> just this, div_by_ or something in include/ .
The cast is not needed, the AND sets to zero all but the low-order 32bits of
divided and then we have the implicit cast.
Are you referring to do_div()? I expect the bitwise operations to be faster.
Bitwise operations are preferred in include/linux/mtd/mtd.h too:
static inline uint32_t mtd_div_by_eb(uint64_t sz, struct mtd_info *mtd)
{
if (mtd->erasesize_shift)
return sz >> mtd->erasesize_shift;
do_div(sz, mtd->erasesize);
return sz;
}
>
>> + return dividend >> erase->size_shift;
>> +}
>> +
>> +static const struct spi_nor_erase_type *
>> +spi_nor_find_best_erase_type(const struct spi_nor_erase_map *map,
>> + const struct spi_nor_erase_region *region,
>> + u64 addr, u32 len)
>> +{
>> + const struct spi_nor_erase_type *erase;
>> + u32 rem;
>> + int i;
>> + u8 erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
>> +
>> + /*
>> + * Erase types are ordered by size, with the biggest erase type at
>> + * index 0.
>> + */
>> + for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
>> + /* Does the erase region support the tested erase type? */
>> + if (!(erase_mask & BIT(i)))
>> + continue;
>> +
>> + erase = &map->erase_type[i];
>> +
>> + /* Don't erase more than what the user has asked for. */
>> + if (erase->size > len)
>> + continue;
>> +
>> + /* Alignment is not mandatory for overlaid regions */
>> + if (region->offset & SNOR_OVERLAID_REGION)
>> + return erase;
>> +
>> + spi_nor_div_by_erase_size(erase, addr, &rem);
>> + if (rem)
>> + continue;
>> + else
>> + return erase;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +static struct spi_nor_erase_region *
>> +spi_nor_region_next(struct spi_nor_erase_region *region)
>> +{
>> + if (spi_nor_region_is_last(region))
>> + return NULL;
>> + return ++region;
>
> region++ ...
It's an array of regions, consecutive in address space, in which walking is done
incrementally. If the received region is not the last, I want to return the next
region, so ++region is correct.
>
> [...]
>
>> +static int spi_nor_cmp_erase_type(const void *a, const void *b)
>> +{
>> + const struct spi_nor_erase_type *erase1 = a;
>> + const struct spi_nor_erase_type *erase2 = b;
>> +
>> + return erase1->size - erase2->size;
>
> What does this function do again ?
It's a compare function, I compare by size the map's Erase Types. I pass a
pointer to this function in the sort() call. I sort in ascending order, by size,
all the map's Erase Types when parsing bfpt. I'm doing the sort at init to speed
up the finding of the best erase command at run-time.
A better name for this function is spi_nor_map_cmp_erase_type(), we compare the
map's Erase Types by size.
>
>> +}
>> +
>> +static void spi_nor_regions_sort_erase_types(struct spi_nor_erase_map *map)
>> +{
>> + struct spi_nor_erase_region *region = map->regions;
>> + struct spi_nor_erase_type *erase_type = map->erase_type;
>> + int i;
>> + u8 region_erase_mask, ordered_erase_mask;
>> +
>> + /*
>> + * Sort each region's Erase Types in ascending order with the smallest
>> + * Erase Type size starting at BIT(0).
>> + */
>> + while (region) {
>> + region_erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
>> +
>> + /*
>> + * The region's erase mask indicates which erase types are
>> + * supported from the erase types defined in the map.
>> + */
>> + ordered_erase_mask = 0;
>> + for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++)
>> + if (erase_type[i].size &&
>> + region_erase_mask & BIT(erase_type[i].idx))
>> + ordered_erase_mask |= BIT(i);
>> +
>> + /* Overwrite erase mask. */
>> + region->offset = (region->offset & ~SNOR_ERASE_TYPE_MASK) |
>> + ordered_erase_mask;
>> +
>> + region = spi_nor_region_next(region);
>> + }
>> +}
>> +
>> +static inline void
>
> Drop the inline
Ok.
>
>> +spi_nor_init_uniform_erase_map(struct spi_nor_erase_map *map,
>> + u8 erase_mask, u64 flash_size)
>> +{
>> + map->uniform_region.offset = SNOR_ERASE_FLAGS_OFFSET(erase_mask, 1, 0,
>> + 0);
>> + map->uniform_region.size = flash_size;
>> + map->regions = &map->uniform_region;
>> + map->uniform_erase_type = erase_mask;
>> +}
>> +
>
> [...]
>
>> +#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)
>
> Get rid of the inlines, really.
Agreed.
>
>> +{
>> + return (region->offset & ~SNOR_ERASE_FLAGS_MASK) + region->size;
>> +}
>> +
>> +static inline bool spi_nor_has_uniform_erase(const struct spi_nor *nor)
>> +{
>> + return !!nor->erase_map.uniform_erase_type;
>> +}
>> +
>> static inline void spi_nor_set_flash_node(struct spi_nor *nor,
>> struct device_node *np)
>> {
>>
>
> General question, what happens if the multi-block erase fails mid-way ,
> is that handled or reported somehow to the user ?
I already implemented your suggestion. I build a list of erase commands to be
executed once I validate that the erase can be performed.
Will send a v3 soon. Cheers,
ta
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RESEND PATCH v2 1/3] mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories
@ 2018-09-07 8:51 ` Tudor Ambarus
0 siblings, 0 replies; 22+ messages in thread
From: Tudor Ambarus @ 2018-09-07 8:51 UTC (permalink / raw)
To: linux-arm-kernel
Thanks Marek,
On 09/03/2018 08:37 PM, Marek Vasut wrote:
> On 08/27/2018 12:26 PM, Tudor Ambarus wrote:
> [...]
>
>> +/* 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_type *erase,
>> + u64 dividend, u32 *remainder)
>> +{
>> + *remainder = (u32)dividend & erase->size_mask;
>
> Is the cast really needed ? btw I think there might be a macro doing
> just this, div_by_ or something in include/ .
The cast is not needed, the AND sets to zero all but the low-order 32bits of
divided and then we have the implicit cast.
Are you referring to do_div()? I expect the bitwise operations to be faster.
Bitwise operations are preferred in include/linux/mtd/mtd.h too:
static inline uint32_t mtd_div_by_eb(uint64_t sz, struct mtd_info *mtd)
{
if (mtd->erasesize_shift)
return sz >> mtd->erasesize_shift;
do_div(sz, mtd->erasesize);
return sz;
}
>
>> + return dividend >> erase->size_shift;
>> +}
>> +
>> +static const struct spi_nor_erase_type *
>> +spi_nor_find_best_erase_type(const struct spi_nor_erase_map *map,
>> + const struct spi_nor_erase_region *region,
>> + u64 addr, u32 len)
>> +{
>> + const struct spi_nor_erase_type *erase;
>> + u32 rem;
>> + int i;
>> + u8 erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
>> +
>> + /*
>> + * Erase types are ordered by size, with the biggest erase type at
>> + * index 0.
>> + */
>> + for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
>> + /* Does the erase region support the tested erase type? */
>> + if (!(erase_mask & BIT(i)))
>> + continue;
>> +
>> + erase = &map->erase_type[i];
>> +
>> + /* Don't erase more than what the user has asked for. */
>> + if (erase->size > len)
>> + continue;
>> +
>> + /* Alignment is not mandatory for overlaid regions */
>> + if (region->offset & SNOR_OVERLAID_REGION)
>> + return erase;
>> +
>> + spi_nor_div_by_erase_size(erase, addr, &rem);
>> + if (rem)
>> + continue;
>> + else
>> + return erase;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +static struct spi_nor_erase_region *
>> +spi_nor_region_next(struct spi_nor_erase_region *region)
>> +{
>> + if (spi_nor_region_is_last(region))
>> + return NULL;
>> + return ++region;
>
> region++ ...
It's an array of regions, consecutive in address space, in which walking is done
incrementally. If the received region is not the last, I want to return the next
region, so ++region is correct.
>
> [...]
>
>> +static int spi_nor_cmp_erase_type(const void *a, const void *b)
>> +{
>> + const struct spi_nor_erase_type *erase1 = a;
>> + const struct spi_nor_erase_type *erase2 = b;
>> +
>> + return erase1->size - erase2->size;
>
> What does this function do again ?
It's a compare function, I compare by size the map's Erase Types. I pass a
pointer to this function in the sort() call. I sort in ascending order, by size,
all the map's Erase Types when parsing bfpt. I'm doing the sort at init to speed
up the finding of the best erase command at run-time.
A better name for this function is spi_nor_map_cmp_erase_type(), we compare the
map's Erase Types by size.
>
>> +}
>> +
>> +static void spi_nor_regions_sort_erase_types(struct spi_nor_erase_map *map)
>> +{
>> + struct spi_nor_erase_region *region = map->regions;
>> + struct spi_nor_erase_type *erase_type = map->erase_type;
>> + int i;
>> + u8 region_erase_mask, ordered_erase_mask;
>> +
>> + /*
>> + * Sort each region's Erase Types in ascending order with the smallest
>> + * Erase Type size starting at BIT(0).
>> + */
>> + while (region) {
>> + region_erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
>> +
>> + /*
>> + * The region's erase mask indicates which erase types are
>> + * supported from the erase types defined in the map.
>> + */
>> + ordered_erase_mask = 0;
>> + for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++)
>> + if (erase_type[i].size &&
>> + region_erase_mask & BIT(erase_type[i].idx))
>> + ordered_erase_mask |= BIT(i);
>> +
>> + /* Overwrite erase mask. */
>> + region->offset = (region->offset & ~SNOR_ERASE_TYPE_MASK) |
>> + ordered_erase_mask;
>> +
>> + region = spi_nor_region_next(region);
>> + }
>> +}
>> +
>> +static inline void
>
> Drop the inline
Ok.
>
>> +spi_nor_init_uniform_erase_map(struct spi_nor_erase_map *map,
>> + u8 erase_mask, u64 flash_size)
>> +{
>> + map->uniform_region.offset = SNOR_ERASE_FLAGS_OFFSET(erase_mask, 1, 0,
>> + 0);
>> + map->uniform_region.size = flash_size;
>> + map->regions = &map->uniform_region;
>> + map->uniform_erase_type = erase_mask;
>> +}
>> +
>
> [...]
>
>> +#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)
>
> Get rid of the inlines, really.
Agreed.
>
>> +{
>> + return (region->offset & ~SNOR_ERASE_FLAGS_MASK) + region->size;
>> +}
>> +
>> +static inline bool spi_nor_has_uniform_erase(const struct spi_nor *nor)
>> +{
>> + return !!nor->erase_map.uniform_erase_type;
>> +}
>> +
>> static inline void spi_nor_set_flash_node(struct spi_nor *nor,
>> struct device_node *np)
>> {
>>
>
> General question, what happens if the multi-block erase fails mid-way ,
> is that handled or reported somehow to the user ?
I already implemented your suggestion. I build a list of erase commands to be
executed once I validate that the erase can be performed.
Will send a v3 soon. Cheers,
ta
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RESEND PATCH v2 2/3] mtd: spi-nor: parse SFDP Sector Map Parameter Table
2018-09-03 17:40 ` Marek Vasut
@ 2018-09-07 9:10 ` Tudor Ambarus
-1 siblings, 0 replies; 22+ messages in thread
From: Tudor Ambarus @ 2018-09-07 9:10 UTC (permalink / raw)
To: Marek Vasut, cyrille.pitchen, dwmw2, computersforpeace,
boris.brezillon, richard
Cc: linux-mtd, linux-arm-kernel, linux-kernel, nicolas.ferre,
Cristian.Birsan
On 09/03/2018 08:40 PM, Marek Vasut wrote:
> On 08/27/2018 12:26 PM, Tudor Ambarus wrote:
> [...]
>> +static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt)
>> +{
>> + const u32 *ret = NULL;
>> + u32 i, addr;
>> + int err;
>> + u8 addr_width, read_opcode, read_dummy;
>> + u8 read_data_mask, data_byte, map_id;
>> +
>> + addr_width = nor->addr_width;
>> + read_dummy = nor->read_dummy;
>> + read_opcode = nor->read_opcode;
>> +
>> + map_id = 0;
>> + i = 0;
>> + /* Determine if there are any optional Detection Command Descriptors */
>> + while (!(smpt[i] & SMPT_DESC_TYPE_MAP)) {
>> + read_data_mask = SMPT_CMD_READ_DATA(smpt[i]);
>> + nor->addr_width = spi_nor_smpt_addr_width(nor, smpt[i]);
>> + nor->read_dummy = spi_nor_smpt_read_dummy(nor, smpt[i]);
>> + nor->read_opcode = SMPT_CMD_OPCODE(smpt[i]);
>> + addr = smpt[i + 1];
>> +
>> + err = spi_nor_read_raw(nor, addr, 1, &data_byte);
>> + if (err)
>> + goto out;
>> +
>> + /*
>> + * Build an index value that is used to select the Sector Map
>> + * Configuration that is currently in use.
>> + */
>> + map_id = map_id << 1 | (!(data_byte & read_data_mask) ? 0 : 1);
>
> You can drop the ternary operator part completely ^
I'll use !! instead.
>
>> + i = i + 2;
>> + }
>> +
>> + /* Find the matching configuration map */
>> + while (SMPT_MAP_ID(smpt[i]) != map_id) {
>> + if (smpt[i] & SMPT_DESC_END)
>> + goto out;
>> + /* increment the table index to the next map */
>> + i += SMPT_MAP_REGION_COUNT(smpt[i]) + 1;
>> + }
>> +
>> + ret = smpt + i;
>> + /* fall through */
>> +out:
>> + nor->addr_width = addr_width;
>> + nor->read_dummy = read_dummy;
>> + nor->read_opcode = read_opcode;
>> + return ret;
>> +}
>> +
>> +static void
>> +spi_nor_region_check_overlay(struct spi_nor_erase_region *region,
>> + const struct spi_nor_erase_type *erase,
>> + const u8 erase_type)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++) {
>> + if (!(erase_type & BIT(i)))
>> + continue;
>> + if (region->size & erase[i].size_mask) {
>> + spi_nor_region_mark_overlay(region);
>> + return;
>> + }
>> + }
>> +}
>> +
>> +static int spi_nor_init_non_uniform_erase_map(struct spi_nor *nor,
>> + const u32 *smpt)
>> +{
>> + struct spi_nor_erase_map *map = &nor->erase_map;
>> + const struct spi_nor_erase_type *erase = map->erase_type;
>> + struct spi_nor_erase_region *region;
>> + u64 offset;
>> + u32 region_count;
>> + int i, j;
>> + u8 erase_type;
>> +
>> + region_count = SMPT_MAP_REGION_COUNT(*smpt);
>> + region = devm_kcalloc(nor->dev, region_count, sizeof(*region),
>> + GFP_KERNEL);
>
> Is this memory always correctly free'd ?
Yes. It will be free'd when the driver detaches from the device.
Thanks,
ta
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RESEND PATCH v2 2/3] mtd: spi-nor: parse SFDP Sector Map Parameter Table
@ 2018-09-07 9:10 ` Tudor Ambarus
0 siblings, 0 replies; 22+ messages in thread
From: Tudor Ambarus @ 2018-09-07 9:10 UTC (permalink / raw)
To: linux-arm-kernel
On 09/03/2018 08:40 PM, Marek Vasut wrote:
> On 08/27/2018 12:26 PM, Tudor Ambarus wrote:
> [...]
>> +static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt)
>> +{
>> + const u32 *ret = NULL;
>> + u32 i, addr;
>> + int err;
>> + u8 addr_width, read_opcode, read_dummy;
>> + u8 read_data_mask, data_byte, map_id;
>> +
>> + addr_width = nor->addr_width;
>> + read_dummy = nor->read_dummy;
>> + read_opcode = nor->read_opcode;
>> +
>> + map_id = 0;
>> + i = 0;
>> + /* Determine if there are any optional Detection Command Descriptors */
>> + while (!(smpt[i] & SMPT_DESC_TYPE_MAP)) {
>> + read_data_mask = SMPT_CMD_READ_DATA(smpt[i]);
>> + nor->addr_width = spi_nor_smpt_addr_width(nor, smpt[i]);
>> + nor->read_dummy = spi_nor_smpt_read_dummy(nor, smpt[i]);
>> + nor->read_opcode = SMPT_CMD_OPCODE(smpt[i]);
>> + addr = smpt[i + 1];
>> +
>> + err = spi_nor_read_raw(nor, addr, 1, &data_byte);
>> + if (err)
>> + goto out;
>> +
>> + /*
>> + * Build an index value that is used to select the Sector Map
>> + * Configuration that is currently in use.
>> + */
>> + map_id = map_id << 1 | (!(data_byte & read_data_mask) ? 0 : 1);
>
> You can drop the ternary operator part completely ^
I'll use !! instead.
>
>> + i = i + 2;
>> + }
>> +
>> + /* Find the matching configuration map */
>> + while (SMPT_MAP_ID(smpt[i]) != map_id) {
>> + if (smpt[i] & SMPT_DESC_END)
>> + goto out;
>> + /* increment the table index to the next map */
>> + i += SMPT_MAP_REGION_COUNT(smpt[i]) + 1;
>> + }
>> +
>> + ret = smpt + i;
>> + /* fall through */
>> +out:
>> + nor->addr_width = addr_width;
>> + nor->read_dummy = read_dummy;
>> + nor->read_opcode = read_opcode;
>> + return ret;
>> +}
>> +
>> +static void
>> +spi_nor_region_check_overlay(struct spi_nor_erase_region *region,
>> + const struct spi_nor_erase_type *erase,
>> + const u8 erase_type)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++) {
>> + if (!(erase_type & BIT(i)))
>> + continue;
>> + if (region->size & erase[i].size_mask) {
>> + spi_nor_region_mark_overlay(region);
>> + return;
>> + }
>> + }
>> +}
>> +
>> +static int spi_nor_init_non_uniform_erase_map(struct spi_nor *nor,
>> + const u32 *smpt)
>> +{
>> + struct spi_nor_erase_map *map = &nor->erase_map;
>> + const struct spi_nor_erase_type *erase = map->erase_type;
>> + struct spi_nor_erase_region *region;
>> + u64 offset;
>> + u32 region_count;
>> + int i, j;
>> + u8 erase_type;
>> +
>> + region_count = SMPT_MAP_REGION_COUNT(*smpt);
>> + region = devm_kcalloc(nor->dev, region_count, sizeof(*region),
>> + GFP_KERNEL);
>
> Is this memory always correctly free'd ?
Yes. It will be free'd when the driver detaches from the device.
Thanks,
ta
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RESEND PATCH v2 1/3] mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories
2018-09-07 8:51 ` Tudor Ambarus
@ 2018-09-07 20:31 ` Marek Vasut
-1 siblings, 0 replies; 22+ messages in thread
From: Marek Vasut @ 2018-09-07 20:31 UTC (permalink / raw)
To: Tudor Ambarus, cyrille.pitchen, dwmw2, computersforpeace,
boris.brezillon, richard
Cc: linux-mtd, linux-arm-kernel, linux-kernel, nicolas.ferre,
Cristian.Birsan
On 09/07/2018 10:51 AM, Tudor Ambarus wrote:
> Thanks Marek,
>
> On 09/03/2018 08:37 PM, Marek Vasut wrote:
>> On 08/27/2018 12:26 PM, Tudor Ambarus wrote:
>> [...]
>>
>>> +/* 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_type *erase,
>>> + u64 dividend, u32 *remainder)
>>> +{
>>> + *remainder = (u32)dividend & erase->size_mask;
>>
>> Is the cast really needed ? btw I think there might be a macro doing
>> just this, div_by_ or something in include/ .
>
> The cast is not needed, the AND sets to zero all but the low-order 32bits of
> divided and then we have the implicit cast.
>
> Are you referring to do_div()? I expect the bitwise operations to be faster.
> Bitwise operations are preferred in include/linux/mtd/mtd.h too:
I thought there was some macro to do this bitwise modulo operation
already , so you don't have to implement it here. I don't mean do_div, no.
> static inline uint32_t mtd_div_by_eb(uint64_t sz, struct mtd_info *mtd)
> {
> if (mtd->erasesize_shift)
> return sz >> mtd->erasesize_shift;
> do_div(sz, mtd->erasesize);
> return sz;
> }
>
>>
>>> + return dividend >> erase->size_shift;
>>> +}
>>> +
>>> +static const struct spi_nor_erase_type *
>>> +spi_nor_find_best_erase_type(const struct spi_nor_erase_map *map,
>>> + const struct spi_nor_erase_region *region,
>>> + u64 addr, u32 len)
>>> +{
>>> + const struct spi_nor_erase_type *erase;
>>> + u32 rem;
>>> + int i;
>>> + u8 erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
>>> +
>>> + /*
>>> + * Erase types are ordered by size, with the biggest erase type at
>>> + * index 0.
>>> + */
>>> + for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
>>> + /* Does the erase region support the tested erase type? */
>>> + if (!(erase_mask & BIT(i)))
>>> + continue;
>>> +
>>> + erase = &map->erase_type[i];
>>> +
>>> + /* Don't erase more than what the user has asked for. */
>>> + if (erase->size > len)
>>> + continue;
>>> +
>>> + /* Alignment is not mandatory for overlaid regions */
>>> + if (region->offset & SNOR_OVERLAID_REGION)
>>> + return erase;
>>> +
>>> + spi_nor_div_by_erase_size(erase, addr, &rem);
>>> + if (rem)
>>> + continue;
>>> + else
>>> + return erase;
>>> + }
>>> +
>>> + return NULL;
>>> +}
>>> +
>>> +static struct spi_nor_erase_region *
>>> +spi_nor_region_next(struct spi_nor_erase_region *region)
>>> +{
>>> + if (spi_nor_region_is_last(region))
>>> + return NULL;
>>> + return ++region;
>>
>> region++ ...
>
> It's an array of regions, consecutive in address space, in which walking is done
> incrementally. If the received region is not the last, I want to return the next
> region, so ++region is correct.
return i++ is the same as return ++i;
>> [...]
>>
>>> +static int spi_nor_cmp_erase_type(const void *a, const void *b)
>>> +{
>>> + const struct spi_nor_erase_type *erase1 = a;
>>> + const struct spi_nor_erase_type *erase2 = b;
>>> +
>>> + return erase1->size - erase2->size;
>>
>> What does this function do again ?
>
> It's a compare function, I compare by size the map's Erase Types. I pass a
> pointer to this function in the sort() call. I sort in ascending order, by size,
> all the map's Erase Types when parsing bfpt. I'm doing the sort at init to speed
> up the finding of the best erase command at run-time.
>
> A better name for this function is spi_nor_map_cmp_erase_type(), we compare the
> map's Erase Types by size.
More like a description would be most welcome, in the actual code. And I
really dislike how you fiddle around with void pointers, can't that be
fixed?
>>> +}
>>> +
>>> +static void spi_nor_regions_sort_erase_types(struct spi_nor_erase_map *map)
>>> +{
>>> + struct spi_nor_erase_region *region = map->regions;
>>> + struct spi_nor_erase_type *erase_type = map->erase_type;
>>> + int i;
>>> + u8 region_erase_mask, ordered_erase_mask;
>>> +
>>> + /*
>>> + * Sort each region's Erase Types in ascending order with the smallest
>>> + * Erase Type size starting at BIT(0).
>>> + */
>>> + while (region) {
>>> + region_erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
>>> +
>>> + /*
>>> + * The region's erase mask indicates which erase types are
>>> + * supported from the erase types defined in the map.
>>> + */
>>> + ordered_erase_mask = 0;
>>> + for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++)
>>> + if (erase_type[i].size &&
>>> + region_erase_mask & BIT(erase_type[i].idx))
>>> + ordered_erase_mask |= BIT(i);
>>> +
>>> + /* Overwrite erase mask. */
>>> + region->offset = (region->offset & ~SNOR_ERASE_TYPE_MASK) |
>>> + ordered_erase_mask;
>>> +
>>> + region = spi_nor_region_next(region);
>>> + }
>>> +}
>>> +
>>> +static inline void
>>
>> Drop the inline
>
> Ok.
>
>>
>>> +spi_nor_init_uniform_erase_map(struct spi_nor_erase_map *map,
>>> + u8 erase_mask, u64 flash_size)
>>> +{
>>> + map->uniform_region.offset = SNOR_ERASE_FLAGS_OFFSET(erase_mask, 1, 0,
>>> + 0);
>>> + map->uniform_region.size = flash_size;
>>> + map->regions = &map->uniform_region;
>>> + map->uniform_erase_type = erase_mask;
>>> +}
>>> +
>>
>> [...]
>>
>>> +#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)
>>
>> Get rid of the inlines, really.
>
> Agreed.
>
>>
>>> +{
>>> + return (region->offset & ~SNOR_ERASE_FLAGS_MASK) + region->size;
>>> +}
>>> +
>>> +static inline bool spi_nor_has_uniform_erase(const struct spi_nor *nor)
>>> +{
>>> + return !!nor->erase_map.uniform_erase_type;
>>> +}
>>> +
>>> static inline void spi_nor_set_flash_node(struct spi_nor *nor,
>>> struct device_node *np)
>>> {
>>>
>>
>> General question, what happens if the multi-block erase fails mid-way ,
>> is that handled or reported somehow to the user ?
>
> I already implemented your suggestion. I build a list of erase commands to be
> executed once I validate that the erase can be performed.
You can send them, but what happens if it fails mid-way ? Is the user
somehow notified that part of his flash is empty and part is not ?
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RESEND PATCH v2 1/3] mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories
@ 2018-09-07 20:31 ` Marek Vasut
0 siblings, 0 replies; 22+ messages in thread
From: Marek Vasut @ 2018-09-07 20:31 UTC (permalink / raw)
To: linux-arm-kernel
On 09/07/2018 10:51 AM, Tudor Ambarus wrote:
> Thanks Marek,
>
> On 09/03/2018 08:37 PM, Marek Vasut wrote:
>> On 08/27/2018 12:26 PM, Tudor Ambarus wrote:
>> [...]
>>
>>> +/* 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_type *erase,
>>> + u64 dividend, u32 *remainder)
>>> +{
>>> + *remainder = (u32)dividend & erase->size_mask;
>>
>> Is the cast really needed ? btw I think there might be a macro doing
>> just this, div_by_ or something in include/ .
>
> The cast is not needed, the AND sets to zero all but the low-order 32bits of
> divided and then we have the implicit cast.
>
> Are you referring to do_div()? I expect the bitwise operations to be faster.
> Bitwise operations are preferred in include/linux/mtd/mtd.h too:
I thought there was some macro to do this bitwise modulo operation
already , so you don't have to implement it here. I don't mean do_div, no.
> static inline uint32_t mtd_div_by_eb(uint64_t sz, struct mtd_info *mtd)
> {
> if (mtd->erasesize_shift)
> return sz >> mtd->erasesize_shift;
> do_div(sz, mtd->erasesize);
> return sz;
> }
>
>>
>>> + return dividend >> erase->size_shift;
>>> +}
>>> +
>>> +static const struct spi_nor_erase_type *
>>> +spi_nor_find_best_erase_type(const struct spi_nor_erase_map *map,
>>> + const struct spi_nor_erase_region *region,
>>> + u64 addr, u32 len)
>>> +{
>>> + const struct spi_nor_erase_type *erase;
>>> + u32 rem;
>>> + int i;
>>> + u8 erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
>>> +
>>> + /*
>>> + * Erase types are ordered by size, with the biggest erase type at
>>> + * index 0.
>>> + */
>>> + for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
>>> + /* Does the erase region support the tested erase type? */
>>> + if (!(erase_mask & BIT(i)))
>>> + continue;
>>> +
>>> + erase = &map->erase_type[i];
>>> +
>>> + /* Don't erase more than what the user has asked for. */
>>> + if (erase->size > len)
>>> + continue;
>>> +
>>> + /* Alignment is not mandatory for overlaid regions */
>>> + if (region->offset & SNOR_OVERLAID_REGION)
>>> + return erase;
>>> +
>>> + spi_nor_div_by_erase_size(erase, addr, &rem);
>>> + if (rem)
>>> + continue;
>>> + else
>>> + return erase;
>>> + }
>>> +
>>> + return NULL;
>>> +}
>>> +
>>> +static struct spi_nor_erase_region *
>>> +spi_nor_region_next(struct spi_nor_erase_region *region)
>>> +{
>>> + if (spi_nor_region_is_last(region))
>>> + return NULL;
>>> + return ++region;
>>
>> region++ ...
>
> It's an array of regions, consecutive in address space, in which walking is done
> incrementally. If the received region is not the last, I want to return the next
> region, so ++region is correct.
return i++ is the same as return ++i;
>> [...]
>>
>>> +static int spi_nor_cmp_erase_type(const void *a, const void *b)
>>> +{
>>> + const struct spi_nor_erase_type *erase1 = a;
>>> + const struct spi_nor_erase_type *erase2 = b;
>>> +
>>> + return erase1->size - erase2->size;
>>
>> What does this function do again ?
>
> It's a compare function, I compare by size the map's Erase Types. I pass a
> pointer to this function in the sort() call. I sort in ascending order, by size,
> all the map's Erase Types when parsing bfpt. I'm doing the sort at init to speed
> up the finding of the best erase command at run-time.
>
> A better name for this function is spi_nor_map_cmp_erase_type(), we compare the
> map's Erase Types by size.
More like a description would be most welcome, in the actual code. And I
really dislike how you fiddle around with void pointers, can't that be
fixed?
>>> +}
>>> +
>>> +static void spi_nor_regions_sort_erase_types(struct spi_nor_erase_map *map)
>>> +{
>>> + struct spi_nor_erase_region *region = map->regions;
>>> + struct spi_nor_erase_type *erase_type = map->erase_type;
>>> + int i;
>>> + u8 region_erase_mask, ordered_erase_mask;
>>> +
>>> + /*
>>> + * Sort each region's Erase Types in ascending order with the smallest
>>> + * Erase Type size starting at BIT(0).
>>> + */
>>> + while (region) {
>>> + region_erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
>>> +
>>> + /*
>>> + * The region's erase mask indicates which erase types are
>>> + * supported from the erase types defined in the map.
>>> + */
>>> + ordered_erase_mask = 0;
>>> + for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++)
>>> + if (erase_type[i].size &&
>>> + region_erase_mask & BIT(erase_type[i].idx))
>>> + ordered_erase_mask |= BIT(i);
>>> +
>>> + /* Overwrite erase mask. */
>>> + region->offset = (region->offset & ~SNOR_ERASE_TYPE_MASK) |
>>> + ordered_erase_mask;
>>> +
>>> + region = spi_nor_region_next(region);
>>> + }
>>> +}
>>> +
>>> +static inline void
>>
>> Drop the inline
>
> Ok.
>
>>
>>> +spi_nor_init_uniform_erase_map(struct spi_nor_erase_map *map,
>>> + u8 erase_mask, u64 flash_size)
>>> +{
>>> + map->uniform_region.offset = SNOR_ERASE_FLAGS_OFFSET(erase_mask, 1, 0,
>>> + 0);
>>> + map->uniform_region.size = flash_size;
>>> + map->regions = &map->uniform_region;
>>> + map->uniform_erase_type = erase_mask;
>>> +}
>>> +
>>
>> [...]
>>
>>> +#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)
>>
>> Get rid of the inlines, really.
>
> Agreed.
>
>>
>>> +{
>>> + return (region->offset & ~SNOR_ERASE_FLAGS_MASK) + region->size;
>>> +}
>>> +
>>> +static inline bool spi_nor_has_uniform_erase(const struct spi_nor *nor)
>>> +{
>>> + return !!nor->erase_map.uniform_erase_type;
>>> +}
>>> +
>>> static inline void spi_nor_set_flash_node(struct spi_nor *nor,
>>> struct device_node *np)
>>> {
>>>
>>
>> General question, what happens if the multi-block erase fails mid-way ,
>> is that handled or reported somehow to the user ?
>
> I already implemented your suggestion. I build a list of erase commands to be
> executed once I validate that the erase can be performed.
You can send them, but what happens if it fails mid-way ? Is the user
somehow notified that part of his flash is empty and part is not ?
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RESEND PATCH v2 1/3] mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories
2018-09-07 20:31 ` Marek Vasut
@ 2018-09-10 10:18 ` Tudor Ambarus
-1 siblings, 0 replies; 22+ messages in thread
From: Tudor Ambarus @ 2018-09-10 10:18 UTC (permalink / raw)
To: Marek Vasut
Cc: cyrille.pitchen, dwmw2, computersforpeace, boris.brezillon,
richard, linux-mtd, linux-arm-kernel, linux-kernel,
nicolas.ferre, Cristian.Birsan
Marek,
On 09/07/2018 11:31 PM, Marek Vasut wrote:
> On 09/07/2018 10:51 AM, Tudor Ambarus wrote:
>> Thanks Marek,
>>
>> On 09/03/2018 08:37 PM, Marek Vasut wrote:
>>> On 08/27/2018 12:26 PM, Tudor Ambarus wrote:
>>> [...]
>>>
>>>> +/* 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_type *erase,
>>>> + u64 dividend, u32 *remainder)
>>>> +{
>>>> + *remainder = (u32)dividend & erase->size_mask;
>>>
>>> Is the cast really needed ? btw I think there might be a macro doing
>>> just this, div_by_ or something in include/ .
>>
>> The cast is not needed, the AND sets to zero all but the low-order 32bits of
>> divided and then we have the implicit cast.
>>
>> Are you referring to do_div()? I expect the bitwise operations to be faster.
>> Bitwise operations are preferred in include/linux/mtd/mtd.h too:
>
> I thought there was some macro to do this bitwise modulo operation
> already , so you don't have to implement it here. I don't mean do_div, no.
>
>> static inline uint32_t mtd_div_by_eb(uint64_t sz, struct mtd_info *mtd)
>> {
>> if (mtd->erasesize_shift)
>> return sz >> mtd->erasesize_shift;
>> do_div(sz, mtd->erasesize);
>> return sz;
>> }
>>
>>>
>>>> + return dividend >> erase->size_shift;
>>>> +}
>>>> +
>>>> +static const struct spi_nor_erase_type *
>>>> +spi_nor_find_best_erase_type(const struct spi_nor_erase_map *map,
>>>> + const struct spi_nor_erase_region *region,
>>>> + u64 addr, u32 len)
>>>> +{
>>>> + const struct spi_nor_erase_type *erase;
>>>> + u32 rem;
>>>> + int i;
>>>> + u8 erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
>>>> +
>>>> + /*
>>>> + * Erase types are ordered by size, with the biggest erase type at
>>>> + * index 0.
>>>> + */
>>>> + for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
>>>> + /* Does the erase region support the tested erase type? */
>>>> + if (!(erase_mask & BIT(i)))
>>>> + continue;
>>>> +
>>>> + erase = &map->erase_type[i];
>>>> +
>>>> + /* Don't erase more than what the user has asked for. */
>>>> + if (erase->size > len)
>>>> + continue;
>>>> +
>>>> + /* Alignment is not mandatory for overlaid regions */
>>>> + if (region->offset & SNOR_OVERLAID_REGION)
>>>> + return erase;
>>>> +
>>>> + spi_nor_div_by_erase_size(erase, addr, &rem);
>>>> + if (rem)
>>>> + continue;
>>>> + else
>>>> + return erase;
>>>> + }
>>>> +
>>>> + return NULL;
>>>> +}
>>>> +
>>>> +static struct spi_nor_erase_region *
>>>> +spi_nor_region_next(struct spi_nor_erase_region *region)
>>>> +{
>>>> + if (spi_nor_region_is_last(region))
>>>> + return NULL;
>>>> + return ++region;
>>>
>>> region++ ...
>>
>> It's an array of regions, consecutive in address space, in which walking is done
>> incrementally. If the received region is not the last, I want to return the next
>> region, so ++region is correct.
>
> return i++ is the same as return ++i;
>
>>> [...]
>>>
>>>> +static int spi_nor_cmp_erase_type(const void *a, const void *b)
>>>> +{
>>>> + const struct spi_nor_erase_type *erase1 = a;
>>>> + const struct spi_nor_erase_type *erase2 = b;
>>>> +
>>>> + return erase1->size - erase2->size;
>>>
>>> What does this function do again ?
>>
>> It's a compare function, I compare by size the map's Erase Types. I pass a
>> pointer to this function in the sort() call. I sort in ascending order, by size,
>> all the map's Erase Types when parsing bfpt. I'm doing the sort at init to speed
>> up the finding of the best erase command at run-time.
>>
>> A better name for this function is spi_nor_map_cmp_erase_type(), we compare the
>> map's Erase Types by size.
>
> More like a description would be most welcome, in the actual code. And I
I will describe all functions, together with arguments and return value.
> really dislike how you fiddle around with void pointers, can't that be
> fixed?
The void pointers are imposed by the sort() declaration, I'm not sure how we can
avoid them. See include/linux/sort.h:
void sort(void *base, size_t num, size_t size,
int (*cmp)(const void *, const void *),
void (*swap)(void *, void *, int));
>
>>>> +}
>>>> +
>>>> +static void spi_nor_regions_sort_erase_types(struct spi_nor_erase_map *map)
>>>> +{
>>>> + struct spi_nor_erase_region *region = map->regions;
>>>> + struct spi_nor_erase_type *erase_type = map->erase_type;
>>>> + int i;
>>>> + u8 region_erase_mask, ordered_erase_mask;
>>>> +
>>>> + /*
>>>> + * Sort each region's Erase Types in ascending order with the smallest
>>>> + * Erase Type size starting at BIT(0).
>>>> + */
>>>> + while (region) {
>>>> + region_erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
>>>> +
>>>> + /*
>>>> + * The region's erase mask indicates which erase types are
>>>> + * supported from the erase types defined in the map.
>>>> + */
>>>> + ordered_erase_mask = 0;
>>>> + for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++)
>>>> + if (erase_type[i].size &&
>>>> + region_erase_mask & BIT(erase_type[i].idx))
>>>> + ordered_erase_mask |= BIT(i);
>>>> +
>>>> + /* Overwrite erase mask. */
>>>> + region->offset = (region->offset & ~SNOR_ERASE_TYPE_MASK) |
>>>> + ordered_erase_mask;
>>>> +
>>>> + region = spi_nor_region_next(region);
>>>> + }
>>>> +}
>>>> +
>>>> +static inline void
>>>
>>> Drop the inline
>>
>> Ok.
>>
>>>
>>>> +spi_nor_init_uniform_erase_map(struct spi_nor_erase_map *map,
>>>> + u8 erase_mask, u64 flash_size)
>>>> +{
>>>> + map->uniform_region.offset = SNOR_ERASE_FLAGS_OFFSET(erase_mask, 1, 0,
>>>> + 0);
>>>> + map->uniform_region.size = flash_size;
>>>> + map->regions = &map->uniform_region;
>>>> + map->uniform_erase_type = erase_mask;
>>>> +}
>>>> +
>>>
>>> [...]
>>>
>>>> +#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)
>>>
>>> Get rid of the inlines, really.
>>
>> Agreed.
>>
>>>
>>>> +{
>>>> + return (region->offset & ~SNOR_ERASE_FLAGS_MASK) + region->size;
>>>> +}
>>>> +
>>>> +static inline bool spi_nor_has_uniform_erase(const struct spi_nor *nor)
>>>> +{
>>>> + return !!nor->erase_map.uniform_erase_type;
>>>> +}
>>>> +
>>>> static inline void spi_nor_set_flash_node(struct spi_nor *nor,
>>>> struct device_node *np)
>>>> {
>>>>
>>>
>>> General question, what happens if the multi-block erase fails mid-way ,
>>> is that handled or reported somehow to the user ?
>>
>> I already implemented your suggestion. I build a list of erase commands to be
>> executed once I validate that the erase can be performed.
>
> You can send them, but what happens if it fails mid-way ? Is the user
> somehow notified that part of his flash is empty and part is not ?
The user will see just the error code, it is not notified which part of the
flash is erased and which not, just like in the uniform erase case. I'm not sure
what would be the advantage of reporting a partial erase. If the erase fails
mid-way then it's not reliable, no?
Since your suggestion also applies for the uniform erase case, would you agree
to let it apart and treat it in a different patch after the non-uniform erase
gets approved?
Thanks,
ta
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RESEND PATCH v2 1/3] mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories
@ 2018-09-10 10:18 ` Tudor Ambarus
0 siblings, 0 replies; 22+ messages in thread
From: Tudor Ambarus @ 2018-09-10 10:18 UTC (permalink / raw)
To: linux-arm-kernel
Marek,
On 09/07/2018 11:31 PM, Marek Vasut wrote:
> On 09/07/2018 10:51 AM, Tudor Ambarus wrote:
>> Thanks Marek,
>>
>> On 09/03/2018 08:37 PM, Marek Vasut wrote:
>>> On 08/27/2018 12:26 PM, Tudor Ambarus wrote:
>>> [...]
>>>
>>>> +/* 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_type *erase,
>>>> + u64 dividend, u32 *remainder)
>>>> +{
>>>> + *remainder = (u32)dividend & erase->size_mask;
>>>
>>> Is the cast really needed ? btw I think there might be a macro doing
>>> just this, div_by_ or something in include/ .
>>
>> The cast is not needed, the AND sets to zero all but the low-order 32bits of
>> divided and then we have the implicit cast.
>>
>> Are you referring to do_div()? I expect the bitwise operations to be faster.
>> Bitwise operations are preferred in include/linux/mtd/mtd.h too:
>
> I thought there was some macro to do this bitwise modulo operation
> already , so you don't have to implement it here. I don't mean do_div, no.
>
>> static inline uint32_t mtd_div_by_eb(uint64_t sz, struct mtd_info *mtd)
>> {
>> if (mtd->erasesize_shift)
>> return sz >> mtd->erasesize_shift;
>> do_div(sz, mtd->erasesize);
>> return sz;
>> }
>>
>>>
>>>> + return dividend >> erase->size_shift;
>>>> +}
>>>> +
>>>> +static const struct spi_nor_erase_type *
>>>> +spi_nor_find_best_erase_type(const struct spi_nor_erase_map *map,
>>>> + const struct spi_nor_erase_region *region,
>>>> + u64 addr, u32 len)
>>>> +{
>>>> + const struct spi_nor_erase_type *erase;
>>>> + u32 rem;
>>>> + int i;
>>>> + u8 erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
>>>> +
>>>> + /*
>>>> + * Erase types are ordered by size, with the biggest erase type at
>>>> + * index 0.
>>>> + */
>>>> + for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
>>>> + /* Does the erase region support the tested erase type? */
>>>> + if (!(erase_mask & BIT(i)))
>>>> + continue;
>>>> +
>>>> + erase = &map->erase_type[i];
>>>> +
>>>> + /* Don't erase more than what the user has asked for. */
>>>> + if (erase->size > len)
>>>> + continue;
>>>> +
>>>> + /* Alignment is not mandatory for overlaid regions */
>>>> + if (region->offset & SNOR_OVERLAID_REGION)
>>>> + return erase;
>>>> +
>>>> + spi_nor_div_by_erase_size(erase, addr, &rem);
>>>> + if (rem)
>>>> + continue;
>>>> + else
>>>> + return erase;
>>>> + }
>>>> +
>>>> + return NULL;
>>>> +}
>>>> +
>>>> +static struct spi_nor_erase_region *
>>>> +spi_nor_region_next(struct spi_nor_erase_region *region)
>>>> +{
>>>> + if (spi_nor_region_is_last(region))
>>>> + return NULL;
>>>> + return ++region;
>>>
>>> region++ ...
>>
>> It's an array of regions, consecutive in address space, in which walking is done
>> incrementally. If the received region is not the last, I want to return the next
>> region, so ++region is correct.
>
> return i++ is the same as return ++i;
>
>>> [...]
>>>
>>>> +static int spi_nor_cmp_erase_type(const void *a, const void *b)
>>>> +{
>>>> + const struct spi_nor_erase_type *erase1 = a;
>>>> + const struct spi_nor_erase_type *erase2 = b;
>>>> +
>>>> + return erase1->size - erase2->size;
>>>
>>> What does this function do again ?
>>
>> It's a compare function, I compare by size the map's Erase Types. I pass a
>> pointer to this function in the sort() call. I sort in ascending order, by size,
>> all the map's Erase Types when parsing bfpt. I'm doing the sort at init to speed
>> up the finding of the best erase command at run-time.
>>
>> A better name for this function is spi_nor_map_cmp_erase_type(), we compare the
>> map's Erase Types by size.
>
> More like a description would be most welcome, in the actual code. And I
I will describe all functions, together with arguments and return value.
> really dislike how you fiddle around with void pointers, can't that be
> fixed?
The void pointers are imposed by the sort() declaration, I'm not sure how we can
avoid them. See include/linux/sort.h:
void sort(void *base, size_t num, size_t size,
int (*cmp)(const void *, const void *),
void (*swap)(void *, void *, int));
>
>>>> +}
>>>> +
>>>> +static void spi_nor_regions_sort_erase_types(struct spi_nor_erase_map *map)
>>>> +{
>>>> + struct spi_nor_erase_region *region = map->regions;
>>>> + struct spi_nor_erase_type *erase_type = map->erase_type;
>>>> + int i;
>>>> + u8 region_erase_mask, ordered_erase_mask;
>>>> +
>>>> + /*
>>>> + * Sort each region's Erase Types in ascending order with the smallest
>>>> + * Erase Type size starting at BIT(0).
>>>> + */
>>>> + while (region) {
>>>> + region_erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
>>>> +
>>>> + /*
>>>> + * The region's erase mask indicates which erase types are
>>>> + * supported from the erase types defined in the map.
>>>> + */
>>>> + ordered_erase_mask = 0;
>>>> + for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++)
>>>> + if (erase_type[i].size &&
>>>> + region_erase_mask & BIT(erase_type[i].idx))
>>>> + ordered_erase_mask |= BIT(i);
>>>> +
>>>> + /* Overwrite erase mask. */
>>>> + region->offset = (region->offset & ~SNOR_ERASE_TYPE_MASK) |
>>>> + ordered_erase_mask;
>>>> +
>>>> + region = spi_nor_region_next(region);
>>>> + }
>>>> +}
>>>> +
>>>> +static inline void
>>>
>>> Drop the inline
>>
>> Ok.
>>
>>>
>>>> +spi_nor_init_uniform_erase_map(struct spi_nor_erase_map *map,
>>>> + u8 erase_mask, u64 flash_size)
>>>> +{
>>>> + map->uniform_region.offset = SNOR_ERASE_FLAGS_OFFSET(erase_mask, 1, 0,
>>>> + 0);
>>>> + map->uniform_region.size = flash_size;
>>>> + map->regions = &map->uniform_region;
>>>> + map->uniform_erase_type = erase_mask;
>>>> +}
>>>> +
>>>
>>> [...]
>>>
>>>> +#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)
>>>
>>> Get rid of the inlines, really.
>>
>> Agreed.
>>
>>>
>>>> +{
>>>> + return (region->offset & ~SNOR_ERASE_FLAGS_MASK) + region->size;
>>>> +}
>>>> +
>>>> +static inline bool spi_nor_has_uniform_erase(const struct spi_nor *nor)
>>>> +{
>>>> + return !!nor->erase_map.uniform_erase_type;
>>>> +}
>>>> +
>>>> static inline void spi_nor_set_flash_node(struct spi_nor *nor,
>>>> struct device_node *np)
>>>> {
>>>>
>>>
>>> General question, what happens if the multi-block erase fails mid-way ,
>>> is that handled or reported somehow to the user ?
>>
>> I already implemented your suggestion. I build a list of erase commands to be
>> executed once I validate that the erase can be performed.
>
> You can send them, but what happens if it fails mid-way ? Is the user
> somehow notified that part of his flash is empty and part is not ?
The user will see just the error code, it is not notified which part of the
flash is erased and which not, just like in the uniform erase case. I'm not sure
what would be the advantage of reporting a partial erase. If the erase fails
mid-way then it's not reliable, no?
Since your suggestion also applies for the uniform erase case, would you agree
to let it apart and treat it in a different patch after the non-uniform erase
gets approved?
Thanks,
ta
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RESEND PATCH v2 1/3] mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories
2018-09-10 10:18 ` Tudor Ambarus
@ 2018-09-10 10:58 ` Marek Vasut
-1 siblings, 0 replies; 22+ messages in thread
From: Marek Vasut @ 2018-09-10 10:58 UTC (permalink / raw)
To: Tudor Ambarus
Cc: cyrille.pitchen, dwmw2, computersforpeace, boris.brezillon,
richard, linux-mtd, linux-arm-kernel, linux-kernel,
nicolas.ferre, Cristian.Birsan
On 09/10/2018 12:18 PM, Tudor Ambarus wrote:
> Marek,
Hi,
> On 09/07/2018 11:31 PM, Marek Vasut wrote:
>> On 09/07/2018 10:51 AM, Tudor Ambarus wrote:
>>> Thanks Marek,
>>>
>>> On 09/03/2018 08:37 PM, Marek Vasut wrote:
>>>> On 08/27/2018 12:26 PM, Tudor Ambarus wrote:
>>>> [...]
>>>>
>>>>> +/* 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_type *erase,
>>>>> + u64 dividend, u32 *remainder)
>>>>> +{
>>>>> + *remainder = (u32)dividend & erase->size_mask;
>>>>
>>>> Is the cast really needed ? btw I think there might be a macro doing
>>>> just this, div_by_ or something in include/ .
>>>
>>> The cast is not needed, the AND sets to zero all but the low-order 32bits of
>>> divided and then we have the implicit cast.
>>>
>>> Are you referring to do_div()? I expect the bitwise operations to be faster.
>>> Bitwise operations are preferred in include/linux/mtd/mtd.h too:
>>
>> I thought there was some macro to do this bitwise modulo operation
>> already , so you don't have to implement it here. I don't mean do_div, no.
>>
>>> static inline uint32_t mtd_div_by_eb(uint64_t sz, struct mtd_info *mtd)
>>> {
>>> if (mtd->erasesize_shift)
>>> return sz >> mtd->erasesize_shift;
>>> do_div(sz, mtd->erasesize);
>>> return sz;
>>> }
>>>
>>>>
>>>>> + return dividend >> erase->size_shift;
>>>>> +}
>>>>> +
>>>>> +static const struct spi_nor_erase_type *
>>>>> +spi_nor_find_best_erase_type(const struct spi_nor_erase_map *map,
>>>>> + const struct spi_nor_erase_region *region,
>>>>> + u64 addr, u32 len)
>>>>> +{
>>>>> + const struct spi_nor_erase_type *erase;
>>>>> + u32 rem;
>>>>> + int i;
>>>>> + u8 erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
>>>>> +
>>>>> + /*
>>>>> + * Erase types are ordered by size, with the biggest erase type at
>>>>> + * index 0.
>>>>> + */
>>>>> + for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
>>>>> + /* Does the erase region support the tested erase type? */
>>>>> + if (!(erase_mask & BIT(i)))
>>>>> + continue;
>>>>> +
>>>>> + erase = &map->erase_type[i];
>>>>> +
>>>>> + /* Don't erase more than what the user has asked for. */
>>>>> + if (erase->size > len)
>>>>> + continue;
>>>>> +
>>>>> + /* Alignment is not mandatory for overlaid regions */
>>>>> + if (region->offset & SNOR_OVERLAID_REGION)
>>>>> + return erase;
>>>>> +
>>>>> + spi_nor_div_by_erase_size(erase, addr, &rem);
>>>>> + if (rem)
>>>>> + continue;
>>>>> + else
>>>>> + return erase;
>>>>> + }
>>>>> +
>>>>> + return NULL;
>>>>> +}
>>>>> +
>>>>> +static struct spi_nor_erase_region *
>>>>> +spi_nor_region_next(struct spi_nor_erase_region *region)
>>>>> +{
>>>>> + if (spi_nor_region_is_last(region))
>>>>> + return NULL;
>>>>> + return ++region;
>>>>
>>>> region++ ...
>>>
>>> It's an array of regions, consecutive in address space, in which walking is done
>>> incrementally. If the received region is not the last, I want to return the next
>>> region, so ++region is correct.
>>
>> return i++ is the same as return ++i;
>>
>>>> [...]
>>>>
>>>>> +static int spi_nor_cmp_erase_type(const void *a, const void *b)
>>>>> +{
>>>>> + const struct spi_nor_erase_type *erase1 = a;
>>>>> + const struct spi_nor_erase_type *erase2 = b;
>>>>> +
>>>>> + return erase1->size - erase2->size;
>>>>
>>>> What does this function do again ?
>>>
>>> It's a compare function, I compare by size the map's Erase Types. I pass a
>>> pointer to this function in the sort() call. I sort in ascending order, by size,
>>> all the map's Erase Types when parsing bfpt. I'm doing the sort at init to speed
>>> up the finding of the best erase command at run-time.
>>>
>>> A better name for this function is spi_nor_map_cmp_erase_type(), we compare the
>>> map's Erase Types by size.
>>
>> More like a description would be most welcome, in the actual code. And I
>
> I will describe all functions, together with arguments and return value.
Thanks !
>> really dislike how you fiddle around with void pointers, can't that be
>> fixed?
>
> The void pointers are imposed by the sort() declaration, I'm not sure how we can
> avoid them. See include/linux/sort.h:
> void sort(void *base, size_t num, size_t size,
> int (*cmp)(const void *, const void *),
> void (*swap)(void *, void *, int));
Ah OK, thanks for clarifying.
>>>>> +
>>>>> +static void spi_nor_regions_sort_erase_types(struct spi_nor_erase_map *map)
>>>>> +{
>>>>> + struct spi_nor_erase_region *region = map->regions;
>>>>> + struct spi_nor_erase_type *erase_type = map->erase_type;
>>>>> + int i;
>>>>> + u8 region_erase_mask, ordered_erase_mask;
>>>>> +
>>>>> + /*
>>>>> + * Sort each region's Erase Types in ascending order with the smallest
>>>>> + * Erase Type size starting at BIT(0).
>>>>> + */
>>>>> + while (region) {
>>>>> + region_erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
>>>>> +
>>>>> + /*
>>>>> + * The region's erase mask indicates which erase types are
>>>>> + * supported from the erase types defined in the map.
>>>>> + */
>>>>> + ordered_erase_mask = 0;
>>>>> + for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++)
>>>>> + if (erase_type[i].size &&
>>>>> + region_erase_mask & BIT(erase_type[i].idx))
>>>>> + ordered_erase_mask |= BIT(i);
>>>>> +
>>>>> + /* Overwrite erase mask. */
>>>>> + region->offset = (region->offset & ~SNOR_ERASE_TYPE_MASK) |
>>>>> + ordered_erase_mask;
>>>>> +
>>>>> + region = spi_nor_region_next(region);
>>>>> + }
>>>>> +}
>>>>> +
>>>>> +static inline void
>>>>
>>>> Drop the inline
>>>
>>> Ok.
>>>
>>>>
>>>>> +spi_nor_init_uniform_erase_map(struct spi_nor_erase_map *map,
>>>>> + u8 erase_mask, u64 flash_size)
>>>>> +{
>>>>> + map->uniform_region.offset = SNOR_ERASE_FLAGS_OFFSET(erase_mask, 1, 0,
>>>>> + 0);
>>>>> + map->uniform_region.size = flash_size;
>>>>> + map->regions = &map->uniform_region;
>>>>> + map->uniform_erase_type = erase_mask;
>>>>> +}
>>>>> +
>>>>
>>>> [...]
>>>>
>>>>> +#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)
>>>>
>>>> Get rid of the inlines, really.
>>>
>>> Agreed.
>>>
>>>>
>>>>> +{
>>>>> + return (region->offset & ~SNOR_ERASE_FLAGS_MASK) + region->size;
>>>>> +}
>>>>> +
>>>>> +static inline bool spi_nor_has_uniform_erase(const struct spi_nor *nor)
>>>>> +{
>>>>> + return !!nor->erase_map.uniform_erase_type;
>>>>> +}
>>>>> +
>>>>> static inline void spi_nor_set_flash_node(struct spi_nor *nor,
>>>>> struct device_node *np)
>>>>> {
>>>>>
>>>>
>>>> General question, what happens if the multi-block erase fails mid-way ,
>>>> is that handled or reported somehow to the user ?
>>>
>>> I already implemented your suggestion. I build a list of erase commands to be
>>> executed once I validate that the erase can be performed.
>>
>> You can send them, but what happens if it fails mid-way ? Is the user
>> somehow notified that part of his flash is empty and part is not ?
>
> The user will see just the error code, it is not notified which part of the
> flash is erased and which not, just like in the uniform erase case. I'm not sure
> what would be the advantage of reporting a partial erase. If the erase fails
> mid-way then it's not reliable, no?
Yes, that's right.
> Since your suggestion also applies for the uniform erase case, would you agree
> to let it apart and treat it in a different patch after the non-uniform erase
> gets approved?
That's fine, just document this behavior please.
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RESEND PATCH v2 1/3] mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories
@ 2018-09-10 10:58 ` Marek Vasut
0 siblings, 0 replies; 22+ messages in thread
From: Marek Vasut @ 2018-09-10 10:58 UTC (permalink / raw)
To: linux-arm-kernel
On 09/10/2018 12:18 PM, Tudor Ambarus wrote:
> Marek,
Hi,
> On 09/07/2018 11:31 PM, Marek Vasut wrote:
>> On 09/07/2018 10:51 AM, Tudor Ambarus wrote:
>>> Thanks Marek,
>>>
>>> On 09/03/2018 08:37 PM, Marek Vasut wrote:
>>>> On 08/27/2018 12:26 PM, Tudor Ambarus wrote:
>>>> [...]
>>>>
>>>>> +/* 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_type *erase,
>>>>> + u64 dividend, u32 *remainder)
>>>>> +{
>>>>> + *remainder = (u32)dividend & erase->size_mask;
>>>>
>>>> Is the cast really needed ? btw I think there might be a macro doing
>>>> just this, div_by_ or something in include/ .
>>>
>>> The cast is not needed, the AND sets to zero all but the low-order 32bits of
>>> divided and then we have the implicit cast.
>>>
>>> Are you referring to do_div()? I expect the bitwise operations to be faster.
>>> Bitwise operations are preferred in include/linux/mtd/mtd.h too:
>>
>> I thought there was some macro to do this bitwise modulo operation
>> already , so you don't have to implement it here. I don't mean do_div, no.
>>
>>> static inline uint32_t mtd_div_by_eb(uint64_t sz, struct mtd_info *mtd)
>>> {
>>> if (mtd->erasesize_shift)
>>> return sz >> mtd->erasesize_shift;
>>> do_div(sz, mtd->erasesize);
>>> return sz;
>>> }
>>>
>>>>
>>>>> + return dividend >> erase->size_shift;
>>>>> +}
>>>>> +
>>>>> +static const struct spi_nor_erase_type *
>>>>> +spi_nor_find_best_erase_type(const struct spi_nor_erase_map *map,
>>>>> + const struct spi_nor_erase_region *region,
>>>>> + u64 addr, u32 len)
>>>>> +{
>>>>> + const struct spi_nor_erase_type *erase;
>>>>> + u32 rem;
>>>>> + int i;
>>>>> + u8 erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
>>>>> +
>>>>> + /*
>>>>> + * Erase types are ordered by size, with the biggest erase type at
>>>>> + * index 0.
>>>>> + */
>>>>> + for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
>>>>> + /* Does the erase region support the tested erase type? */
>>>>> + if (!(erase_mask & BIT(i)))
>>>>> + continue;
>>>>> +
>>>>> + erase = &map->erase_type[i];
>>>>> +
>>>>> + /* Don't erase more than what the user has asked for. */
>>>>> + if (erase->size > len)
>>>>> + continue;
>>>>> +
>>>>> + /* Alignment is not mandatory for overlaid regions */
>>>>> + if (region->offset & SNOR_OVERLAID_REGION)
>>>>> + return erase;
>>>>> +
>>>>> + spi_nor_div_by_erase_size(erase, addr, &rem);
>>>>> + if (rem)
>>>>> + continue;
>>>>> + else
>>>>> + return erase;
>>>>> + }
>>>>> +
>>>>> + return NULL;
>>>>> +}
>>>>> +
>>>>> +static struct spi_nor_erase_region *
>>>>> +spi_nor_region_next(struct spi_nor_erase_region *region)
>>>>> +{
>>>>> + if (spi_nor_region_is_last(region))
>>>>> + return NULL;
>>>>> + return ++region;
>>>>
>>>> region++ ...
>>>
>>> It's an array of regions, consecutive in address space, in which walking is done
>>> incrementally. If the received region is not the last, I want to return the next
>>> region, so ++region is correct.
>>
>> return i++ is the same as return ++i;
>>
>>>> [...]
>>>>
>>>>> +static int spi_nor_cmp_erase_type(const void *a, const void *b)
>>>>> +{
>>>>> + const struct spi_nor_erase_type *erase1 = a;
>>>>> + const struct spi_nor_erase_type *erase2 = b;
>>>>> +
>>>>> + return erase1->size - erase2->size;
>>>>
>>>> What does this function do again ?
>>>
>>> It's a compare function, I compare by size the map's Erase Types. I pass a
>>> pointer to this function in the sort() call. I sort in ascending order, by size,
>>> all the map's Erase Types when parsing bfpt. I'm doing the sort at init to speed
>>> up the finding of the best erase command at run-time.
>>>
>>> A better name for this function is spi_nor_map_cmp_erase_type(), we compare the
>>> map's Erase Types by size.
>>
>> More like a description would be most welcome, in the actual code. And I
>
> I will describe all functions, together with arguments and return value.
Thanks !
>> really dislike how you fiddle around with void pointers, can't that be
>> fixed?
>
> The void pointers are imposed by the sort() declaration, I'm not sure how we can
> avoid them. See include/linux/sort.h:
> void sort(void *base, size_t num, size_t size,
> int (*cmp)(const void *, const void *),
> void (*swap)(void *, void *, int));
Ah OK, thanks for clarifying.
>>>>> +
>>>>> +static void spi_nor_regions_sort_erase_types(struct spi_nor_erase_map *map)
>>>>> +{
>>>>> + struct spi_nor_erase_region *region = map->regions;
>>>>> + struct spi_nor_erase_type *erase_type = map->erase_type;
>>>>> + int i;
>>>>> + u8 region_erase_mask, ordered_erase_mask;
>>>>> +
>>>>> + /*
>>>>> + * Sort each region's Erase Types in ascending order with the smallest
>>>>> + * Erase Type size starting at BIT(0).
>>>>> + */
>>>>> + while (region) {
>>>>> + region_erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
>>>>> +
>>>>> + /*
>>>>> + * The region's erase mask indicates which erase types are
>>>>> + * supported from the erase types defined in the map.
>>>>> + */
>>>>> + ordered_erase_mask = 0;
>>>>> + for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++)
>>>>> + if (erase_type[i].size &&
>>>>> + region_erase_mask & BIT(erase_type[i].idx))
>>>>> + ordered_erase_mask |= BIT(i);
>>>>> +
>>>>> + /* Overwrite erase mask. */
>>>>> + region->offset = (region->offset & ~SNOR_ERASE_TYPE_MASK) |
>>>>> + ordered_erase_mask;
>>>>> +
>>>>> + region = spi_nor_region_next(region);
>>>>> + }
>>>>> +}
>>>>> +
>>>>> +static inline void
>>>>
>>>> Drop the inline
>>>
>>> Ok.
>>>
>>>>
>>>>> +spi_nor_init_uniform_erase_map(struct spi_nor_erase_map *map,
>>>>> + u8 erase_mask, u64 flash_size)
>>>>> +{
>>>>> + map->uniform_region.offset = SNOR_ERASE_FLAGS_OFFSET(erase_mask, 1, 0,
>>>>> + 0);
>>>>> + map->uniform_region.size = flash_size;
>>>>> + map->regions = &map->uniform_region;
>>>>> + map->uniform_erase_type = erase_mask;
>>>>> +}
>>>>> +
>>>>
>>>> [...]
>>>>
>>>>> +#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)
>>>>
>>>> Get rid of the inlines, really.
>>>
>>> Agreed.
>>>
>>>>
>>>>> +{
>>>>> + return (region->offset & ~SNOR_ERASE_FLAGS_MASK) + region->size;
>>>>> +}
>>>>> +
>>>>> +static inline bool spi_nor_has_uniform_erase(const struct spi_nor *nor)
>>>>> +{
>>>>> + return !!nor->erase_map.uniform_erase_type;
>>>>> +}
>>>>> +
>>>>> static inline void spi_nor_set_flash_node(struct spi_nor *nor,
>>>>> struct device_node *np)
>>>>> {
>>>>>
>>>>
>>>> General question, what happens if the multi-block erase fails mid-way ,
>>>> is that handled or reported somehow to the user ?
>>>
>>> I already implemented your suggestion. I build a list of erase commands to be
>>> executed once I validate that the erase can be performed.
>>
>> You can send them, but what happens if it fails mid-way ? Is the user
>> somehow notified that part of his flash is empty and part is not ?
>
> The user will see just the error code, it is not notified which part of the
> flash is erased and which not, just like in the uniform erase case. I'm not sure
> what would be the advantage of reporting a partial erase. If the erase fails
> mid-way then it's not reliable, no?
Yes, that's right.
> Since your suggestion also applies for the uniform erase case, would you agree
> to let it apart and treat it in a different patch after the non-uniform erase
> gets approved?
That's fine, just document this behavior please.
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2018-09-10 10:58 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-27 10:26 [RESEND PATCH v2 0/3] add support to non-uniform SFDP SPI NOR flash memories Tudor Ambarus
2018-08-27 10:26 ` Tudor Ambarus
2018-08-27 10:26 ` [RESEND PATCH v2 1/3] mtd: spi-nor: " Tudor Ambarus
2018-08-27 10:26 ` Tudor Ambarus
2018-09-03 17:37 ` Marek Vasut
2018-09-03 17:37 ` Marek Vasut
2018-09-07 8:51 ` Tudor Ambarus
2018-09-07 8:51 ` Tudor Ambarus
2018-09-07 20:31 ` Marek Vasut
2018-09-07 20:31 ` Marek Vasut
2018-09-10 10:18 ` Tudor Ambarus
2018-09-10 10:18 ` Tudor Ambarus
2018-09-10 10:58 ` Marek Vasut
2018-09-10 10:58 ` Marek Vasut
2018-08-27 10:26 ` [RESEND PATCH v2 2/3] mtd: spi-nor: parse SFDP Sector Map Parameter Table Tudor Ambarus
2018-08-27 10:26 ` Tudor Ambarus
2018-09-03 17:40 ` Marek Vasut
2018-09-03 17:40 ` Marek Vasut
2018-09-07 9:10 ` Tudor Ambarus
2018-09-07 9:10 ` Tudor Ambarus
2018-08-27 10:26 ` [RESEND PATCH v2 3/3] mtd: spi-nor: parse SFDP 4-byte Address Instruction Table Tudor Ambarus
2018-08-27 10:26 ` Tudor Ambarus
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.