* [PATCH v15 1/8] mtd: spi-nor: s/addr_width/addr_nbytes
2022-05-09 22:10 [PATCH v15 0/8] mtd: spi-nor: Add support for Infineon s25hl-t/s25hs-t tkuw584924
@ 2022-05-09 22:10 ` tkuw584924
2022-05-12 21:01 ` Michael Walle
2022-05-31 11:13 ` Pratyush Yadav
2022-05-09 22:10 ` [PATCH v15 2/8] mtd: spi-nor: core: Shrink the storage size of the flash_info's addr_nbytes tkuw584924
` (6 subsequent siblings)
7 siblings, 2 replies; 42+ messages in thread
From: tkuw584924 @ 2022-05-09 22:10 UTC (permalink / raw)
To: linux-mtd
Cc: tudor.ambarus, miquel.raynal, richard, vigneshr, p.yadav,
michael, tkuw584924, Bacem.Daassi
From: Tudor Ambarus <tudor.ambarus@microchip.com>
Address width was an unfortunate name, as it means the number of IO lines
used for the address, whereas in the code it is used as the number of
address bytes. s/addr_width/addr_nbytes throughout the entire SPI NOR
framework.
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
drivers/mtd/spi-nor/controllers/aspeed-smc.c | 6 +--
drivers/mtd/spi-nor/controllers/hisi-sfc.c | 2 +-
drivers/mtd/spi-nor/controllers/nxp-spifi.c | 8 ++--
drivers/mtd/spi-nor/core.c | 46 ++++++++++----------
drivers/mtd/spi-nor/core.h | 12 ++---
drivers/mtd/spi-nor/issi.c | 2 +-
drivers/mtd/spi-nor/otp.c | 12 ++---
drivers/mtd/spi-nor/sfdp.c | 32 +++++++-------
drivers/mtd/spi-nor/xilinx.c | 2 +-
include/linux/mtd/spi-nor.h | 4 +-
10 files changed, 63 insertions(+), 63 deletions(-)
diff --git a/drivers/mtd/spi-nor/controllers/aspeed-smc.c b/drivers/mtd/spi-nor/controllers/aspeed-smc.c
index acfe010f9dd7..5ac8c321535b 100644
--- a/drivers/mtd/spi-nor/controllers/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/controllers/aspeed-smc.c
@@ -350,10 +350,10 @@ static void aspeed_smc_send_cmd_addr(struct spi_nor *nor, u8 cmd, u32 addr)
__be32 temp;
u32 cmdaddr;
- switch (nor->addr_width) {
+ switch (nor->addr_nbytes) {
default:
WARN_ONCE(1, "Unexpected address width %u, defaulting to 3\n",
- nor->addr_width);
+ nor->addr_nbytes);
fallthrough;
case 3:
cmdaddr = addr & 0xFFFFFF;
@@ -709,7 +709,7 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
const struct aspeed_smc_info *info = controller->info;
u32 cmd;
- if (chip->nor.addr_width == 4 && info->set_4b)
+ if (chip->nor.addr_nbytes == 4 && info->set_4b)
info->set_4b(chip);
/* This is for direct AHB access when using Command Mode. */
diff --git a/drivers/mtd/spi-nor/controllers/hisi-sfc.c b/drivers/mtd/spi-nor/controllers/hisi-sfc.c
index 94a969185ceb..5070d72835ec 100644
--- a/drivers/mtd/spi-nor/controllers/hisi-sfc.c
+++ b/drivers/mtd/spi-nor/controllers/hisi-sfc.c
@@ -237,7 +237,7 @@ static int hisi_spi_nor_dma_transfer(struct spi_nor *nor, loff_t start_off,
reg = readl(host->regbase + FMC_CFG);
reg &= ~(FMC_CFG_OP_MODE_MASK | SPI_NOR_ADDR_MODE_MASK);
reg |= FMC_CFG_OP_MODE_NORMAL;
- reg |= (nor->addr_width == 4) ? SPI_NOR_ADDR_MODE_4BYTES
+ reg |= (nor->addr_nbytes == 4) ? SPI_NOR_ADDR_MODE_4BYTES
: SPI_NOR_ADDR_MODE_3BYTES;
writel(reg, host->regbase + FMC_CFG);
diff --git a/drivers/mtd/spi-nor/controllers/nxp-spifi.c b/drivers/mtd/spi-nor/controllers/nxp-spifi.c
index 9032b9ab2eaf..ab3990e6ac25 100644
--- a/drivers/mtd/spi-nor/controllers/nxp-spifi.c
+++ b/drivers/mtd/spi-nor/controllers/nxp-spifi.c
@@ -203,7 +203,7 @@ static ssize_t nxp_spifi_write(struct spi_nor *nor, loff_t to, size_t len,
SPIFI_CMD_DATALEN(len) |
SPIFI_CMD_FIELDFORM_ALL_SERIAL |
SPIFI_CMD_OPCODE(nor->program_opcode) |
- SPIFI_CMD_FRAMEFORM(spifi->nor.addr_width + 1);
+ SPIFI_CMD_FRAMEFORM(spifi->nor.addr_nbytes + 1);
writel(cmd, spifi->io_base + SPIFI_CMD);
for (i = 0; i < len; i++)
@@ -230,7 +230,7 @@ static int nxp_spifi_erase(struct spi_nor *nor, loff_t offs)
cmd = SPIFI_CMD_FIELDFORM_ALL_SERIAL |
SPIFI_CMD_OPCODE(nor->erase_opcode) |
- SPIFI_CMD_FRAMEFORM(spifi->nor.addr_width + 1);
+ SPIFI_CMD_FRAMEFORM(spifi->nor.addr_nbytes + 1);
writel(cmd, spifi->io_base + SPIFI_CMD);
return nxp_spifi_wait_for_cmd(spifi);
@@ -252,12 +252,12 @@ static int nxp_spifi_setup_memory_cmd(struct nxp_spifi *spifi)
}
/* Memory mode supports address length between 1 and 4 */
- if (spifi->nor.addr_width < 1 || spifi->nor.addr_width > 4)
+ if (spifi->nor.addr_nbytes < 1 || spifi->nor.addr_nbytes > 4)
return -EINVAL;
spifi->mcmd |= SPIFI_CMD_OPCODE(spifi->nor.read_opcode) |
SPIFI_CMD_INTLEN(spifi->nor.read_dummy / 8) |
- SPIFI_CMD_FRAMEFORM(spifi->nor.addr_width + 1);
+ SPIFI_CMD_FRAMEFORM(spifi->nor.addr_nbytes + 1);
return 0;
}
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 2bfa84100d38..7db6b41d7c30 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -38,7 +38,7 @@
*/
#define CHIP_ERASE_2MB_READY_WAIT_JIFFIES (40UL * HZ)
-#define SPI_NOR_MAX_ADDR_WIDTH 4
+#define SPI_NOR_MAX_ADDR_NBYTES 4
#define SPI_NOR_SRST_SLEEP_MIN 200
#define SPI_NOR_SRST_SLEEP_MAX 400
@@ -198,7 +198,7 @@ static ssize_t spi_nor_spimem_read_data(struct spi_nor *nor, loff_t from,
{
struct spi_mem_op op =
SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 0),
- SPI_MEM_OP_ADDR(nor->addr_width, from, 0),
+ SPI_MEM_OP_ADDR(nor->addr_nbytes, from, 0),
SPI_MEM_OP_DUMMY(nor->read_dummy, 0),
SPI_MEM_OP_DATA_IN(len, buf, 0));
bool usebouncebuf;
@@ -262,7 +262,7 @@ static ssize_t spi_nor_spimem_write_data(struct spi_nor *nor, loff_t to,
{
struct spi_mem_op op =
SPI_MEM_OP(SPI_MEM_OP_CMD(nor->program_opcode, 0),
- SPI_MEM_OP_ADDR(nor->addr_width, to, 0),
+ SPI_MEM_OP_ADDR(nor->addr_nbytes, to, 0),
SPI_MEM_OP_NO_DUMMY,
SPI_MEM_OP_DATA_OUT(len, buf, 0));
ssize_t nbytes;
@@ -1134,7 +1134,7 @@ int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
if (nor->spimem) {
struct spi_mem_op op =
SPI_NOR_SECTOR_ERASE_OP(nor->erase_opcode,
- nor->addr_width, addr);
+ nor->addr_nbytes, addr);
spi_nor_spimem_setup_op(nor, &op, nor->write_proto);
@@ -1147,13 +1147,13 @@ int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
* Default implementation, if driver doesn't have a specialized HW
* control
*/
- for (i = nor->addr_width - 1; i >= 0; i--) {
+ for (i = nor->addr_nbytes - 1; i >= 0; i--) {
nor->bouncebuf[i] = addr & 0xff;
addr >>= 8;
}
return spi_nor_controller_ops_write_reg(nor, nor->erase_opcode,
- nor->bouncebuf, nor->addr_width);
+ nor->bouncebuf, nor->addr_nbytes);
}
/**
@@ -2270,9 +2270,9 @@ static int spi_nor_default_setup(struct spi_nor *nor,
return 0;
}
-static int spi_nor_set_addr_width(struct spi_nor *nor)
+static int spi_nor_set_addr_nbytes(struct spi_nor *nor)
{
- if (nor->addr_width) {
+ if (nor->addr_nbytes) {
/* already configured from SFDP */
} else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
/*
@@ -2287,26 +2287,26 @@ static int spi_nor_set_addr_width(struct spi_nor *nor)
* Force all 8D-8D-8D flashes to use an address width of 4 to
* avoid this situation.
*/
- nor->addr_width = 4;
- } else if (nor->info->addr_width) {
- nor->addr_width = nor->info->addr_width;
+ nor->addr_nbytes = 4;
+ } else if (nor->info->addr_nbytes) {
+ nor->addr_nbytes = nor->info->addr_nbytes;
} else {
- nor->addr_width = 3;
+ nor->addr_nbytes = 3;
}
- if (nor->addr_width == 3 && nor->params->size > 0x1000000) {
+ if (nor->addr_nbytes == 3 && nor->params->size > 0x1000000) {
/* enable 4-byte addressing if the device exceeds 16MiB */
- nor->addr_width = 4;
+ nor->addr_nbytes = 4;
}
- if (nor->addr_width > SPI_NOR_MAX_ADDR_WIDTH) {
+ if (nor->addr_nbytes > SPI_NOR_MAX_ADDR_NBYTES) {
dev_dbg(nor->dev, "address width is too large: %u\n",
- nor->addr_width);
+ nor->addr_nbytes);
return -EINVAL;
}
/* Set 4byte opcodes when possible. */
- if (nor->addr_width == 4 && nor->flags & SNOR_F_4B_OPCODES &&
+ if (nor->addr_nbytes == 4 && nor->flags & SNOR_F_4B_OPCODES &&
!(nor->flags & SNOR_F_HAS_4BAIT))
spi_nor_set_4byte_opcodes(nor);
@@ -2325,7 +2325,7 @@ static int spi_nor_setup(struct spi_nor *nor,
if (ret)
return ret;
- return spi_nor_set_addr_width(nor);
+ return spi_nor_set_addr_nbytes(nor);
}
/**
@@ -2518,7 +2518,7 @@ static void spi_nor_sfdp_init_params_deprecated(struct spi_nor *nor)
if (spi_nor_parse_sfdp(nor)) {
memcpy(nor->params, &sfdp_params, sizeof(*nor->params));
- nor->addr_width = 0;
+ nor->addr_nbytes = 0;
nor->flags &= ~SNOR_F_4B_OPCODES;
}
}
@@ -2739,7 +2739,7 @@ static int spi_nor_init(struct spi_nor *nor)
nor->flags & SNOR_F_SWP_IS_VOLATILE))
spi_nor_try_unlock_all(nor);
- if (nor->addr_width == 4 &&
+ if (nor->addr_nbytes == 4 &&
nor->read_proto != SNOR_PROTO_8_8_8_DTR &&
!(nor->flags & SNOR_F_4B_OPCODES)) {
/*
@@ -2866,7 +2866,7 @@ static void spi_nor_put_device(struct mtd_info *mtd)
void spi_nor_restore(struct spi_nor *nor)
{
/* restore the addressing mode */
- if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES) &&
+ if (nor->addr_nbytes == 4 && !(nor->flags & SNOR_F_4B_OPCODES) &&
nor->flags & SNOR_F_BROKEN_RESET)
nor->params->set_4byte_addr_mode(nor, false);
@@ -3056,7 +3056,7 @@ static int spi_nor_create_read_dirmap(struct spi_nor *nor)
{
struct spi_mem_dirmap_info info = {
.op_tmpl = SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 0),
- SPI_MEM_OP_ADDR(nor->addr_width, 0, 0),
+ SPI_MEM_OP_ADDR(nor->addr_nbytes, 0, 0),
SPI_MEM_OP_DUMMY(nor->read_dummy, 0),
SPI_MEM_OP_DATA_IN(0, NULL, 0)),
.offset = 0,
@@ -3087,7 +3087,7 @@ static int spi_nor_create_write_dirmap(struct spi_nor *nor)
{
struct spi_mem_dirmap_info info = {
.op_tmpl = SPI_MEM_OP(SPI_MEM_OP_CMD(nor->program_opcode, 0),
- SPI_MEM_OP_ADDR(nor->addr_width, 0, 0),
+ SPI_MEM_OP_ADDR(nor->addr_nbytes, 0, 0),
SPI_MEM_OP_NO_DUMMY,
SPI_MEM_OP_DATA_OUT(0, NULL, 0)),
.offset = 0,
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 80d4831a4f34..5fc3d4a8add1 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -90,9 +90,9 @@
SPI_MEM_OP_NO_DUMMY, \
SPI_MEM_OP_NO_DATA)
-#define SPI_NOR_SECTOR_ERASE_OP(opcode, addr_width, addr) \
+#define SPI_NOR_SECTOR_ERASE_OP(opcode, addr_nbytes, addr) \
SPI_MEM_OP(SPI_MEM_OP_CMD(opcode, 0), \
- SPI_MEM_OP_ADDR(addr_width, addr, 0), \
+ SPI_MEM_OP_ADDR(addr_nbytes, addr, 0), \
SPI_MEM_OP_NO_DUMMY, \
SPI_MEM_OP_NO_DATA)
@@ -434,7 +434,7 @@ struct spi_nor_fixups {
* isn't necessarily called a "sector" by the vendor.
* @n_sectors: the number of sectors.
* @page_size: the flash's page size.
- * @addr_width: the flash's address width.
+ * @addr_nbytes: number of address bytes to send.
*
* @parse_sfdp: true when flash supports SFDP tables. The false value has no
* meaning. If one wants to skip the SFDP tables, one should
@@ -493,7 +493,7 @@ struct flash_info {
unsigned sector_size;
u16 n_sectors;
u16 page_size;
- u16 addr_width;
+ u16 addr_nbytes;
bool parse_sfdp;
u16 flags;
@@ -555,11 +555,11 @@ struct flash_info {
.n_sectors = (_n_sectors), \
.page_size = 256, \
-#define CAT25_INFO(_sector_size, _n_sectors, _page_size, _addr_width) \
+#define CAT25_INFO(_sector_size, _n_sectors, _page_size, _addr_nbytes) \
.sector_size = (_sector_size), \
.n_sectors = (_n_sectors), \
.page_size = (_page_size), \
- .addr_width = (_addr_width), \
+ .addr_nbytes = (_addr_nbytes), \
.flags = SPI_NOR_NO_ERASE | SPI_NOR_NO_FR, \
#define OTP_INFO(_len, _n_regions, _base, _offset) \
diff --git a/drivers/mtd/spi-nor/issi.c b/drivers/mtd/spi-nor/issi.c
index c012bc2486e1..23e331cc42a9 100644
--- a/drivers/mtd/spi-nor/issi.c
+++ b/drivers/mtd/spi-nor/issi.c
@@ -20,7 +20,7 @@ is25lp256_post_bfpt_fixups(struct spi_nor *nor,
*/
if ((bfpt->dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) ==
BFPT_DWORD1_ADDRESS_BYTES_3_ONLY)
- nor->addr_width = 4;
+ nor->addr_nbytes = 4;
return 0;
}
diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
index fa63d8571218..00ab0d2d6d2f 100644
--- a/drivers/mtd/spi-nor/otp.c
+++ b/drivers/mtd/spi-nor/otp.c
@@ -35,13 +35,13 @@
*/
int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf)
{
- u8 addr_width, read_opcode, read_dummy;
+ u8 addr_nbytes, read_opcode, read_dummy;
struct spi_mem_dirmap_desc *rdesc;
enum spi_nor_protocol read_proto;
int ret;
read_opcode = nor->read_opcode;
- addr_width = nor->addr_width;
+ addr_nbytes = nor->addr_nbytes;
read_dummy = nor->read_dummy;
read_proto = nor->read_proto;
rdesc = nor->dirmap.rdesc;
@@ -54,7 +54,7 @@ int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf)
ret = spi_nor_read_data(nor, addr, len, buf);
nor->read_opcode = read_opcode;
- nor->addr_width = addr_width;
+ nor->addr_nbytes = addr_nbytes;
nor->read_dummy = read_dummy;
nor->read_proto = read_proto;
nor->dirmap.rdesc = rdesc;
@@ -85,11 +85,11 @@ int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
{
enum spi_nor_protocol write_proto;
struct spi_mem_dirmap_desc *wdesc;
- u8 addr_width, program_opcode;
+ u8 addr_nbytes, program_opcode;
int ret, written;
program_opcode = nor->program_opcode;
- addr_width = nor->addr_width;
+ addr_nbytes = nor->addr_nbytes;
write_proto = nor->write_proto;
wdesc = nor->dirmap.wdesc;
@@ -113,7 +113,7 @@ int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
out:
nor->program_opcode = program_opcode;
- nor->addr_width = addr_width;
+ nor->addr_nbytes = addr_nbytes;
nor->write_proto = write_proto;
nor->dirmap.wdesc = wdesc;
diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index a5211543d30d..61ae8c8c5237 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -134,7 +134,7 @@ struct sfdp_4bait {
/**
* spi_nor_read_raw() - raw read of serial flash memory. read_opcode,
- * addr_width and read_dummy members of the struct spi_nor
+ * addr_nbytes and read_dummy members of the struct spi_nor
* should be previously
* set.
* @nor: pointer to a 'struct spi_nor'
@@ -178,21 +178,21 @@ static int spi_nor_read_raw(struct spi_nor *nor, u32 addr, size_t len, u8 *buf)
static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
size_t len, void *buf)
{
- u8 addr_width, read_opcode, read_dummy;
+ u8 addr_nbytes, read_opcode, read_dummy;
int ret;
read_opcode = nor->read_opcode;
- addr_width = nor->addr_width;
+ addr_nbytes = nor->addr_nbytes;
read_dummy = nor->read_dummy;
nor->read_opcode = SPINOR_OP_RDSFDP;
- nor->addr_width = 3;
+ nor->addr_nbytes = 3;
nor->read_dummy = 8;
ret = spi_nor_read_raw(nor, addr, len, buf);
nor->read_opcode = read_opcode;
- nor->addr_width = addr_width;
+ nor->addr_nbytes = addr_nbytes;
nor->read_dummy = read_dummy;
return ret;
@@ -462,11 +462,11 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) {
case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY:
case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4:
- nor->addr_width = 3;
+ nor->addr_nbytes = 3;
break;
case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY:
- nor->addr_width = 4;
+ nor->addr_nbytes = 4;
break;
default:
@@ -637,12 +637,12 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
}
/**
- * spi_nor_smpt_addr_width() - return the address width used in the
+ * spi_nor_smpt_addr_nbytes() - return the number of address bytes used in the
* configuration detection command.
* @nor: pointer to a 'struct spi_nor'
* @settings: configuration detection command descriptor, dword1
*/
-static u8 spi_nor_smpt_addr_width(const struct spi_nor *nor, const u32 settings)
+static u8 spi_nor_smpt_addr_nbytes(const struct spi_nor *nor, const u32 settings)
{
switch (settings & SMPT_CMD_ADDRESS_LEN_MASK) {
case SMPT_CMD_ADDRESS_LEN_0:
@@ -653,7 +653,7 @@ static u8 spi_nor_smpt_addr_width(const struct spi_nor *nor, const u32 settings)
return 4;
case SMPT_CMD_ADDRESS_LEN_USE_CURRENT:
default:
- return nor->addr_width;
+ return nor->addr_nbytes;
}
}
@@ -690,7 +690,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
u32 addr;
int err;
u8 i;
- u8 addr_width, read_opcode, read_dummy;
+ u8 addr_nbytes, read_opcode, read_dummy;
u8 read_data_mask, map_id;
/* Use a kmalloc'ed bounce buffer to guarantee it is DMA-able. */
@@ -698,7 +698,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
if (!buf)
return ERR_PTR(-ENOMEM);
- addr_width = nor->addr_width;
+ addr_nbytes = nor->addr_nbytes;
read_dummy = nor->read_dummy;
read_opcode = nor->read_opcode;
@@ -709,7 +709,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
break;
read_data_mask = SMPT_CMD_READ_DATA(smpt[i]);
- nor->addr_width = spi_nor_smpt_addr_width(nor, smpt[i]);
+ nor->addr_nbytes = spi_nor_smpt_addr_nbytes(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];
@@ -756,7 +756,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
/* fall through */
out:
kfree(buf);
- nor->addr_width = addr_width;
+ nor->addr_nbytes = addr_nbytes;
nor->read_dummy = read_dummy;
nor->read_opcode = read_opcode;
return ret;
@@ -1044,7 +1044,7 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
/*
* 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.
+ * nor->addr_nbytes value.
*/
if (!read_hwcaps || !pp_hwcaps || !erase_mask)
goto out;
@@ -1098,7 +1098,7 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
* Spansion memory. However this quirk is no longer needed with new
* SFDP compliant memories.
*/
- nor->addr_width = 4;
+ nor->addr_nbytes = 4;
nor->flags |= SNOR_F_4B_OPCODES | SNOR_F_HAS_4BAIT;
/* fall through */
diff --git a/drivers/mtd/spi-nor/xilinx.c b/drivers/mtd/spi-nor/xilinx.c
index 1d2f5db047bd..5723157739fc 100644
--- a/drivers/mtd/spi-nor/xilinx.c
+++ b/drivers/mtd/spi-nor/xilinx.c
@@ -31,7 +31,7 @@
.sector_size = (8 * (_page_size)), \
.n_sectors = (_n_sectors), \
.page_size = (_page_size), \
- .addr_width = 3, \
+ .addr_nbytes = 3, \
.flags = SPI_NOR_NO_FR
/* Xilinx S3AN share MFR with Atmel SPI NOR */
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 5e25a7b75ae2..07b1324d434b 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -353,7 +353,7 @@ struct spi_nor_flash_parameter;
* @bouncebuf_size: size of the bounce buffer
* @info: SPI NOR part JEDEC MFR ID and other info
* @manufacturer: SPI NOR manufacturer
- * @addr_width: number of address bytes
+ * @addr_nbytes: number of address bytes
* @erase_opcode: the opcode for erasing a sector
* @read_opcode: the read opcode
* @read_dummy: the dummy needed by the read operation
@@ -382,7 +382,7 @@ struct spi_nor {
size_t bouncebuf_size;
const struct flash_info *info;
const struct spi_nor_manufacturer *manufacturer;
- u8 addr_width;
+ u8 addr_nbytes;
u8 erase_opcode;
u8 read_opcode;
u8 read_dummy;
--
2.25.1
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v15 1/8] mtd: spi-nor: s/addr_width/addr_nbytes
2022-05-09 22:10 ` [PATCH v15 1/8] mtd: spi-nor: s/addr_width/addr_nbytes tkuw584924
@ 2022-05-12 21:01 ` Michael Walle
2022-05-31 11:13 ` Pratyush Yadav
1 sibling, 0 replies; 42+ messages in thread
From: Michael Walle @ 2022-05-12 21:01 UTC (permalink / raw)
To: tkuw584924
Cc: linux-mtd, tudor.ambarus, miquel.raynal, richard, vigneshr,
p.yadav, Bacem.Daassi
Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>
> Address width was an unfortunate name, as it means the number of IO
> lines
> used for the address, whereas in the code it is used as the number of
> address bytes. s/addr_width/addr_nbytes throughout the entire SPI NOR
> framework.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
I'm not a big fan of just renaming, but
Reviewed-by: Michael Walle <michael@walle.cc>
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 1/8] mtd: spi-nor: s/addr_width/addr_nbytes
2022-05-09 22:10 ` [PATCH v15 1/8] mtd: spi-nor: s/addr_width/addr_nbytes tkuw584924
2022-05-12 21:01 ` Michael Walle
@ 2022-05-31 11:13 ` Pratyush Yadav
1 sibling, 0 replies; 42+ messages in thread
From: Pratyush Yadav @ 2022-05-31 11:13 UTC (permalink / raw)
To: tkuw584924
Cc: linux-mtd, tudor.ambarus, miquel.raynal, richard, vigneshr,
michael, Bacem.Daassi
On 10/05/22 07:10AM, tkuw584924@gmail.com wrote:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>
> Address width was an unfortunate name, as it means the number of IO lines
> used for the address, whereas in the code it is used as the number of
> address bytes. s/addr_width/addr_nbytes throughout the entire SPI NOR
> framework.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
> drivers/mtd/spi-nor/controllers/aspeed-smc.c | 6 +--
> drivers/mtd/spi-nor/controllers/hisi-sfc.c | 2 +-
> drivers/mtd/spi-nor/controllers/nxp-spifi.c | 8 ++--
> drivers/mtd/spi-nor/core.c | 46 ++++++++++----------
> drivers/mtd/spi-nor/core.h | 12 ++---
> drivers/mtd/spi-nor/issi.c | 2 +-
> drivers/mtd/spi-nor/otp.c | 12 ++---
> drivers/mtd/spi-nor/sfdp.c | 32 +++++++-------
> drivers/mtd/spi-nor/xilinx.c | 2 +-
> include/linux/mtd/spi-nor.h | 4 +-
> 10 files changed, 63 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/controllers/aspeed-smc.c b/drivers/mtd/spi-nor/controllers/aspeed-smc.c
> index acfe010f9dd7..5ac8c321535b 100644
> --- a/drivers/mtd/spi-nor/controllers/aspeed-smc.c
> +++ b/drivers/mtd/spi-nor/controllers/aspeed-smc.c
> @@ -350,10 +350,10 @@ static void aspeed_smc_send_cmd_addr(struct spi_nor *nor, u8 cmd, u32 addr)
> __be32 temp;
> u32 cmdaddr;
>
> - switch (nor->addr_width) {
> + switch (nor->addr_nbytes) {
> default:
> WARN_ONCE(1, "Unexpected address width %u, defaulting to 3\n",
s/width/nbytes/ here as well.
> - nor->addr_width);
> + nor->addr_nbytes);
> fallthrough;
> case 3:
> cmdaddr = addr & 0xFFFFFF;
> @@ -709,7 +709,7 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
> const struct aspeed_smc_info *info = controller->info;
> u32 cmd;
>
> - if (chip->nor.addr_width == 4 && info->set_4b)
> + if (chip->nor.addr_nbytes == 4 && info->set_4b)
> info->set_4b(chip);
>
> /* This is for direct AHB access when using Command Mode. */
> diff --git a/drivers/mtd/spi-nor/controllers/hisi-sfc.c b/drivers/mtd/spi-nor/controllers/hisi-sfc.c
> index 94a969185ceb..5070d72835ec 100644
> --- a/drivers/mtd/spi-nor/controllers/hisi-sfc.c
> +++ b/drivers/mtd/spi-nor/controllers/hisi-sfc.c
> @@ -237,7 +237,7 @@ static int hisi_spi_nor_dma_transfer(struct spi_nor *nor, loff_t start_off,
> reg = readl(host->regbase + FMC_CFG);
> reg &= ~(FMC_CFG_OP_MODE_MASK | SPI_NOR_ADDR_MODE_MASK);
> reg |= FMC_CFG_OP_MODE_NORMAL;
> - reg |= (nor->addr_width == 4) ? SPI_NOR_ADDR_MODE_4BYTES
> + reg |= (nor->addr_nbytes == 4) ? SPI_NOR_ADDR_MODE_4BYTES
> : SPI_NOR_ADDR_MODE_3BYTES;
> writel(reg, host->regbase + FMC_CFG);
>
> diff --git a/drivers/mtd/spi-nor/controllers/nxp-spifi.c b/drivers/mtd/spi-nor/controllers/nxp-spifi.c
> index 9032b9ab2eaf..ab3990e6ac25 100644
> --- a/drivers/mtd/spi-nor/controllers/nxp-spifi.c
> +++ b/drivers/mtd/spi-nor/controllers/nxp-spifi.c
> @@ -203,7 +203,7 @@ static ssize_t nxp_spifi_write(struct spi_nor *nor, loff_t to, size_t len,
> SPIFI_CMD_DATALEN(len) |
> SPIFI_CMD_FIELDFORM_ALL_SERIAL |
> SPIFI_CMD_OPCODE(nor->program_opcode) |
> - SPIFI_CMD_FRAMEFORM(spifi->nor.addr_width + 1);
> + SPIFI_CMD_FRAMEFORM(spifi->nor.addr_nbytes + 1);
> writel(cmd, spifi->io_base + SPIFI_CMD);
>
> for (i = 0; i < len; i++)
> @@ -230,7 +230,7 @@ static int nxp_spifi_erase(struct spi_nor *nor, loff_t offs)
>
> cmd = SPIFI_CMD_FIELDFORM_ALL_SERIAL |
> SPIFI_CMD_OPCODE(nor->erase_opcode) |
> - SPIFI_CMD_FRAMEFORM(spifi->nor.addr_width + 1);
> + SPIFI_CMD_FRAMEFORM(spifi->nor.addr_nbytes + 1);
> writel(cmd, spifi->io_base + SPIFI_CMD);
>
> return nxp_spifi_wait_for_cmd(spifi);
> @@ -252,12 +252,12 @@ static int nxp_spifi_setup_memory_cmd(struct nxp_spifi *spifi)
> }
>
> /* Memory mode supports address length between 1 and 4 */
> - if (spifi->nor.addr_width < 1 || spifi->nor.addr_width > 4)
> + if (spifi->nor.addr_nbytes < 1 || spifi->nor.addr_nbytes > 4)
> return -EINVAL;
>
> spifi->mcmd |= SPIFI_CMD_OPCODE(spifi->nor.read_opcode) |
> SPIFI_CMD_INTLEN(spifi->nor.read_dummy / 8) |
> - SPIFI_CMD_FRAMEFORM(spifi->nor.addr_width + 1);
> + SPIFI_CMD_FRAMEFORM(spifi->nor.addr_nbytes + 1);
>
> return 0;
> }
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 2bfa84100d38..7db6b41d7c30 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -38,7 +38,7 @@
> */
> #define CHIP_ERASE_2MB_READY_WAIT_JIFFIES (40UL * HZ)
>
> -#define SPI_NOR_MAX_ADDR_WIDTH 4
> +#define SPI_NOR_MAX_ADDR_NBYTES 4
>
> #define SPI_NOR_SRST_SLEEP_MIN 200
> #define SPI_NOR_SRST_SLEEP_MAX 400
> @@ -198,7 +198,7 @@ static ssize_t spi_nor_spimem_read_data(struct spi_nor *nor, loff_t from,
> {
> struct spi_mem_op op =
> SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 0),
> - SPI_MEM_OP_ADDR(nor->addr_width, from, 0),
> + SPI_MEM_OP_ADDR(nor->addr_nbytes, from, 0),
I am guessing this patch no longer applies since c0abb861c5d0 ("mtd:
spi-nor: Introduce templates for SPI NOR operations"). Patch looks good
otherwise.
Acked-by: Pratyush Yadav <p.yadav@ti.com>
[...]
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v15 2/8] mtd: spi-nor: core: Shrink the storage size of the flash_info's addr_nbytes
2022-05-09 22:10 [PATCH v15 0/8] mtd: spi-nor: Add support for Infineon s25hl-t/s25hs-t tkuw584924
2022-05-09 22:10 ` [PATCH v15 1/8] mtd: spi-nor: s/addr_width/addr_nbytes tkuw584924
@ 2022-05-09 22:10 ` tkuw584924
2022-05-12 21:22 ` Michael Walle
2022-05-31 11:14 ` Pratyush Yadav
2022-05-09 22:10 ` [PATCH v15 3/8] mtd: spi-nor: sfdp: Clarify that nor->read_{opcode, dummy} are uninitialized tkuw584924
` (5 subsequent siblings)
7 siblings, 2 replies; 42+ messages in thread
From: tkuw584924 @ 2022-05-09 22:10 UTC (permalink / raw)
To: linux-mtd
Cc: tudor.ambarus, miquel.raynal, richard, vigneshr, p.yadav,
michael, tkuw584924, Bacem.Daassi
From: Tudor Ambarus <tudor.ambarus@microchip.com>
The maximum number of address bytes in SPI NOR is 4. Shrink the storage
size of the flash_info's addr_nbytes.
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
drivers/mtd/spi-nor/core.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 5fc3d4a8add1..fe7683fe1b4d 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -493,7 +493,7 @@ struct flash_info {
unsigned sector_size;
u16 n_sectors;
u16 page_size;
- u16 addr_nbytes;
+ u8 addr_nbytes;
bool parse_sfdp;
u16 flags;
--
2.25.1
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v15 2/8] mtd: spi-nor: core: Shrink the storage size of the flash_info's addr_nbytes
2022-05-09 22:10 ` [PATCH v15 2/8] mtd: spi-nor: core: Shrink the storage size of the flash_info's addr_nbytes tkuw584924
@ 2022-05-12 21:22 ` Michael Walle
2022-05-31 11:14 ` Pratyush Yadav
1 sibling, 0 replies; 42+ messages in thread
From: Michael Walle @ 2022-05-12 21:22 UTC (permalink / raw)
To: tkuw584924
Cc: linux-mtd, tudor.ambarus, miquel.raynal, richard, vigneshr,
p.yadav, Bacem.Daassi
Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>
> The maximum number of address bytes in SPI NOR is 4. Shrink the storage
> size of the flash_info's addr_nbytes.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
I'm not even sure if this actually shrink the size due to
alignments of the members by the compiler, nor do I see any
value in saving one byte. But anyway:
Reviewed-by: Michael Walle <michael@walle.cc>
-michael
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 2/8] mtd: spi-nor: core: Shrink the storage size of the flash_info's addr_nbytes
2022-05-09 22:10 ` [PATCH v15 2/8] mtd: spi-nor: core: Shrink the storage size of the flash_info's addr_nbytes tkuw584924
2022-05-12 21:22 ` Michael Walle
@ 2022-05-31 11:14 ` Pratyush Yadav
1 sibling, 0 replies; 42+ messages in thread
From: Pratyush Yadav @ 2022-05-31 11:14 UTC (permalink / raw)
To: tkuw584924
Cc: linux-mtd, tudor.ambarus, miquel.raynal, richard, vigneshr,
michael, Bacem.Daassi
On 10/05/22 07:10AM, tkuw584924@gmail.com wrote:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>
> The maximum number of address bytes in SPI NOR is 4. Shrink the storage
> size of the flash_info's addr_nbytes.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v15 3/8] mtd: spi-nor: sfdp: Clarify that nor->read_{opcode, dummy} are uninitialized
2022-05-09 22:10 [PATCH v15 0/8] mtd: spi-nor: Add support for Infineon s25hl-t/s25hs-t tkuw584924
2022-05-09 22:10 ` [PATCH v15 1/8] mtd: spi-nor: s/addr_width/addr_nbytes tkuw584924
2022-05-09 22:10 ` [PATCH v15 2/8] mtd: spi-nor: core: Shrink the storage size of the flash_info's addr_nbytes tkuw584924
@ 2022-05-09 22:10 ` tkuw584924
2022-05-12 21:33 ` Michael Walle
2022-05-31 11:18 ` Pratyush Yadav
2022-05-09 22:10 ` [PATCH v15 4/8] mtd: spi-nor: Do not change nor->addr_nbytes at SFDP parsing time tkuw584924
` (4 subsequent siblings)
7 siblings, 2 replies; 42+ messages in thread
From: tkuw584924 @ 2022-05-09 22:10 UTC (permalink / raw)
To: linux-mtd
Cc: tudor.ambarus, miquel.raynal, richard, vigneshr, p.yadav,
michael, tkuw584924, Bacem.Daassi, Takahiro Kuwano
From: Tudor Ambarus <tudor.ambarus@microchip.com>
nor->read_{opcode, dummy} are uninitialized (value zero) at SFDP parsing
time. Clarify that in the code.
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
Tested-By: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
drivers/mtd/spi-nor/sfdp.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index 61ae8c8c5237..058ce218d2af 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -178,12 +178,10 @@ static int spi_nor_read_raw(struct spi_nor *nor, u32 addr, size_t len, u8 *buf)
static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
size_t len, void *buf)
{
- u8 addr_nbytes, read_opcode, read_dummy;
+ u8 addr_nbytes;
int ret;
- read_opcode = nor->read_opcode;
addr_nbytes = nor->addr_nbytes;
- read_dummy = nor->read_dummy;
nor->read_opcode = SPINOR_OP_RDSFDP;
nor->addr_nbytes = 3;
@@ -191,9 +189,10 @@ static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
ret = spi_nor_read_raw(nor, addr, len, buf);
- nor->read_opcode = read_opcode;
nor->addr_nbytes = addr_nbytes;
- nor->read_dummy = read_dummy;
+ /* Restore setup to its uninitialized state. */
+ nor->read_opcode = 0;
+ nor->read_dummy = 0;
return ret;
}
@@ -690,7 +689,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
u32 addr;
int err;
u8 i;
- u8 addr_nbytes, read_opcode, read_dummy;
+ u8 addr_nbytes;
u8 read_data_mask, map_id;
/* Use a kmalloc'ed bounce buffer to guarantee it is DMA-able. */
@@ -699,8 +698,6 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
return ERR_PTR(-ENOMEM);
addr_nbytes = nor->addr_nbytes;
- read_dummy = nor->read_dummy;
- read_opcode = nor->read_opcode;
map_id = 0;
/* Determine if there are any optional Detection Command Descriptors */
@@ -757,8 +754,9 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
out:
kfree(buf);
nor->addr_nbytes = addr_nbytes;
- nor->read_dummy = read_dummy;
- nor->read_opcode = read_opcode;
+ /* Restore setup to its uninitialized state. */
+ nor->read_dummy = 0;
+ nor->read_opcode = 0;
return ret;
}
--
2.25.1
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v15 3/8] mtd: spi-nor: sfdp: Clarify that nor->read_{opcode, dummy} are uninitialized
2022-05-09 22:10 ` [PATCH v15 3/8] mtd: spi-nor: sfdp: Clarify that nor->read_{opcode, dummy} are uninitialized tkuw584924
@ 2022-05-12 21:33 ` Michael Walle
2022-05-12 21:38 ` Michael Walle
2022-05-31 11:18 ` Pratyush Yadav
1 sibling, 1 reply; 42+ messages in thread
From: Michael Walle @ 2022-05-12 21:33 UTC (permalink / raw)
To: tkuw584924
Cc: linux-mtd, tudor.ambarus, miquel.raynal, richard, vigneshr,
p.yadav, Bacem.Daassi, Takahiro Kuwano
Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>
> nor->read_{opcode, dummy} are uninitialized (value zero) at SFDP
> parsing
> time. Clarify that in the code.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> Tested-By: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
> drivers/mtd/spi-nor/sfdp.c | 18 ++++++++----------
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index 61ae8c8c5237..058ce218d2af 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -178,12 +178,10 @@ static int spi_nor_read_raw(struct spi_nor *nor,
> u32 addr, size_t len, u8 *buf)
> static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
> size_t len, void *buf)
> {
> - u8 addr_nbytes, read_opcode, read_dummy;
> + u8 addr_nbytes;
> int ret;
>
> - read_opcode = nor->read_opcode;
> addr_nbytes = nor->addr_nbytes;
> - read_dummy = nor->read_dummy;
>
> nor->read_opcode = SPINOR_OP_RDSFDP;
> nor->addr_nbytes = 3;
> @@ -191,9 +189,10 @@ static int spi_nor_read_sfdp(struct spi_nor *nor,
> u32 addr,
>
> ret = spi_nor_read_raw(nor, addr, len, buf);
>
> - nor->read_opcode = read_opcode;
> nor->addr_nbytes = addr_nbytes;
> - nor->read_dummy = read_dummy;
> + /* Restore setup to its uninitialized state. */
> + nor->read_opcode = 0;
> + nor->read_dummy = 0;
>
> return ret;
> }
> @@ -690,7 +689,7 @@ static const u32 *spi_nor_get_map_in_use(struct
> spi_nor *nor, const u32 *smpt,
> u32 addr;
> int err;
> u8 i;
> - u8 addr_nbytes, read_opcode, read_dummy;
> + u8 addr_nbytes;
> u8 read_data_mask, map_id;
>
> /* Use a kmalloc'ed bounce buffer to guarantee it is DMA-able. */
> @@ -699,8 +698,6 @@ static const u32 *spi_nor_get_map_in_use(struct
> spi_nor *nor, const u32 *smpt,
> return ERR_PTR(-ENOMEM);
>
> addr_nbytes = nor->addr_nbytes;
> - read_dummy = nor->read_dummy;
> - read_opcode = nor->read_opcode;
>
> map_id = 0;
> /* Determine if there are any optional Detection Command Descriptors
> */
> @@ -757,8 +754,9 @@ static const u32 *spi_nor_get_map_in_use(struct
> spi_nor *nor, const u32 *smpt,
> out:
> kfree(buf);
> nor->addr_nbytes = addr_nbytes;
> - nor->read_dummy = read_dummy;
> - nor->read_opcode = read_opcode;
> + /* Restore setup to its uninitialized state. */
Who is modifying this? FWIW, I don't like this because it's now
different than all the other occurrences of this pattern.
Ultimately, I'd like to get rid of all these
saved_params = params;
params = new_params;
spi_nor_call()
params = saved_params;
And instead use something like
__spi_nor_call(non_default_params);
-michael
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v15 3/8] mtd: spi-nor: sfdp: Clarify that nor->read_{opcode, dummy} are uninitialized
2022-05-12 21:33 ` Michael Walle
@ 2022-05-12 21:38 ` Michael Walle
0 siblings, 0 replies; 42+ messages in thread
From: Michael Walle @ 2022-05-12 21:38 UTC (permalink / raw)
To: tkuw584924
Cc: linux-mtd, tudor.ambarus, miquel.raynal, richard, vigneshr,
p.yadav, Bacem.Daassi, Takahiro Kuwano
Am 2022-05-12 23:33, schrieb Michael Walle:
> Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
>> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>>
>> nor->read_{opcode, dummy} are uninitialized (value zero) at SFDP
>> parsing
>> time. Clarify that in the code.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> Tested-By: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> ---
>> drivers/mtd/spi-nor/sfdp.c | 18 ++++++++----------
>> 1 file changed, 8 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>> index 61ae8c8c5237..058ce218d2af 100644
>> --- a/drivers/mtd/spi-nor/sfdp.c
>> +++ b/drivers/mtd/spi-nor/sfdp.c
>> @@ -178,12 +178,10 @@ static int spi_nor_read_raw(struct spi_nor *nor,
>> u32 addr, size_t len, u8 *buf)
>> static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
>> size_t len, void *buf)
>> {
>> - u8 addr_nbytes, read_opcode, read_dummy;
>> + u8 addr_nbytes;
>> int ret;
>>
>> - read_opcode = nor->read_opcode;
>> addr_nbytes = nor->addr_nbytes;
>> - read_dummy = nor->read_dummy;
>>
>> nor->read_opcode = SPINOR_OP_RDSFDP;
>> nor->addr_nbytes = 3;
>> @@ -191,9 +189,10 @@ static int spi_nor_read_sfdp(struct spi_nor *nor,
>> u32 addr,
>>
>> ret = spi_nor_read_raw(nor, addr, len, buf);
>>
>> - nor->read_opcode = read_opcode;
>> nor->addr_nbytes = addr_nbytes;
>> - nor->read_dummy = read_dummy;
>> + /* Restore setup to its uninitialized state. */
>> + nor->read_opcode = 0;
>> + nor->read_dummy = 0;
>>
>> return ret;
>> }
>> @@ -690,7 +689,7 @@ static const u32 *spi_nor_get_map_in_use(struct
>> spi_nor *nor, const u32 *smpt,
>> u32 addr;
>> int err;
>> u8 i;
>> - u8 addr_nbytes, read_opcode, read_dummy;
>> + u8 addr_nbytes;
>> u8 read_data_mask, map_id;
>>
>> /* Use a kmalloc'ed bounce buffer to guarantee it is DMA-able. */
>> @@ -699,8 +698,6 @@ static const u32 *spi_nor_get_map_in_use(struct
>> spi_nor *nor, const u32 *smpt,
>> return ERR_PTR(-ENOMEM);
>>
>> addr_nbytes = nor->addr_nbytes;
>> - read_dummy = nor->read_dummy;
>> - read_opcode = nor->read_opcode;
>>
>> map_id = 0;
>> /* Determine if there are any optional Detection Command Descriptors
>> */
>> @@ -757,8 +754,9 @@ static const u32 *spi_nor_get_map_in_use(struct
>> spi_nor *nor, const u32 *smpt,
>> out:
>> kfree(buf);
>> nor->addr_nbytes = addr_nbytes;
>> - nor->read_dummy = read_dummy;
>> - nor->read_opcode = read_opcode;
>> + /* Restore setup to its uninitialized state. */
>
> Who is modifying this?
Ahh scrap that, this very function. I need to go to sleep.
I just saw your next patch which will also get rid of the
addr_nbytes. I'd still like to get rid of this state sometime
and pass it as an additional parameter. But:
Reviewed-by: Michael Walle <michael@walle.cc>
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 3/8] mtd: spi-nor: sfdp: Clarify that nor->read_{opcode, dummy} are uninitialized
2022-05-09 22:10 ` [PATCH v15 3/8] mtd: spi-nor: sfdp: Clarify that nor->read_{opcode, dummy} are uninitialized tkuw584924
2022-05-12 21:33 ` Michael Walle
@ 2022-05-31 11:18 ` Pratyush Yadav
1 sibling, 0 replies; 42+ messages in thread
From: Pratyush Yadav @ 2022-05-31 11:18 UTC (permalink / raw)
To: tkuw584924
Cc: linux-mtd, tudor.ambarus, miquel.raynal, richard, vigneshr,
michael, Bacem.Daassi, Takahiro Kuwano
On 10/05/22 07:10AM, tkuw584924@gmail.com wrote:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>
> nor->read_{opcode, dummy} are uninitialized (value zero) at SFDP parsing
> time. Clarify that in the code.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> Tested-By: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
> drivers/mtd/spi-nor/sfdp.c | 18 ++++++++----------
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index 61ae8c8c5237..058ce218d2af 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -178,12 +178,10 @@ static int spi_nor_read_raw(struct spi_nor *nor, u32 addr, size_t len, u8 *buf)
> static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
> size_t len, void *buf)
> {
> - u8 addr_nbytes, read_opcode, read_dummy;
> + u8 addr_nbytes;
> int ret;
>
> - read_opcode = nor->read_opcode;
> addr_nbytes = nor->addr_nbytes;
> - read_dummy = nor->read_dummy;
>
> nor->read_opcode = SPINOR_OP_RDSFDP;
> nor->addr_nbytes = 3;
> @@ -191,9 +189,10 @@ static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
>
> ret = spi_nor_read_raw(nor, addr, len, buf);
>
> - nor->read_opcode = read_opcode;
> nor->addr_nbytes = addr_nbytes;
> - nor->read_dummy = read_dummy;
> + /* Restore setup to its uninitialized state. */
> + nor->read_opcode = 0;
> + nor->read_dummy = 0;
Hmm, I don't like this too much. Why do you need to explicitly set the
default value here? What's wrong with this function just not caring
about what is initialized by this point and what is not? It is really
not its business caring about default values of these parameters. Modify
them, run the command, and restore them, regardless of default or
initialized values.
>
> return ret;
> }
> @@ -690,7 +689,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
> u32 addr;
> int err;
> u8 i;
> - u8 addr_nbytes, read_opcode, read_dummy;
> + u8 addr_nbytes;
> u8 read_data_mask, map_id;
>
> /* Use a kmalloc'ed bounce buffer to guarantee it is DMA-able. */
> @@ -699,8 +698,6 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
> return ERR_PTR(-ENOMEM);
>
> addr_nbytes = nor->addr_nbytes;
> - read_dummy = nor->read_dummy;
> - read_opcode = nor->read_opcode;
>
> map_id = 0;
> /* Determine if there are any optional Detection Command Descriptors */
> @@ -757,8 +754,9 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
> out:
> kfree(buf);
> nor->addr_nbytes = addr_nbytes;
> - nor->read_dummy = read_dummy;
> - nor->read_opcode = read_opcode;
> + /* Restore setup to its uninitialized state. */
> + nor->read_dummy = 0;
> + nor->read_opcode = 0;
> return ret;
> }
>
> --
> 2.25.1
>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v15 4/8] mtd: spi-nor: Do not change nor->addr_nbytes at SFDP parsing time
2022-05-09 22:10 [PATCH v15 0/8] mtd: spi-nor: Add support for Infineon s25hl-t/s25hs-t tkuw584924
` (2 preceding siblings ...)
2022-05-09 22:10 ` [PATCH v15 3/8] mtd: spi-nor: sfdp: Clarify that nor->read_{opcode, dummy} are uninitialized tkuw584924
@ 2022-05-09 22:10 ` tkuw584924
2022-05-12 21:45 ` Michael Walle
2022-05-31 11:30 ` Pratyush Yadav
2022-05-09 22:10 ` [PATCH v15 5/8] mtd: spi-nor: core: Couple the number of address bytes with the address mode tkuw584924
` (3 subsequent siblings)
7 siblings, 2 replies; 42+ messages in thread
From: tkuw584924 @ 2022-05-09 22:10 UTC (permalink / raw)
To: linux-mtd
Cc: tudor.ambarus, miquel.raynal, richard, vigneshr, p.yadav,
michael, tkuw584924, Bacem.Daassi, Takahiro Kuwano
From: Tudor Ambarus <tudor.ambarus@microchip.com>
At the SFDP parsing time we should not change members of struct spi_nor,
but instead fill members of struct spi_nor_flash_parameters which could
leter on be used by callers. The caller will then decide if SFDP params
should be used and more importantly when they should be used. Clean the
code flow and don't initialize nor->addr_nbytes at SFDP parsing time.
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
Tested-By: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
drivers/mtd/spi-nor/core.c | 5 ++---
drivers/mtd/spi-nor/core.h | 2 ++
drivers/mtd/spi-nor/sfdp.c | 18 ++++++------------
3 files changed, 10 insertions(+), 15 deletions(-)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 7db6b41d7c30..dd71deba9f11 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2272,8 +2272,8 @@ static int spi_nor_default_setup(struct spi_nor *nor,
static int spi_nor_set_addr_nbytes(struct spi_nor *nor)
{
- if (nor->addr_nbytes) {
- /* already configured from SFDP */
+ if (nor->params->addr_nbytes) {
+ nor->addr_nbytes = nor->params->addr_nbytes;
} else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
/*
* In 8D-8D-8D mode, one byte takes half a cycle to transfer. So
@@ -2518,7 +2518,6 @@ static void spi_nor_sfdp_init_params_deprecated(struct spi_nor *nor)
if (spi_nor_parse_sfdp(nor)) {
memcpy(nor->params, &sfdp_params, sizeof(*nor->params));
- nor->addr_nbytes = 0;
nor->flags &= ~SNOR_F_4B_OPCODES;
}
}
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index fe7683fe1b4d..41df8bc8e008 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -345,6 +345,7 @@ struct spi_nor_otp {
* @writesize Minimal writable flash unit size. Defaults to 1. Set to
* ECC unit size for ECC-ed flashes.
* @page_size: the page size of the SPI NOR flash memory.
+ * @addr_nbytes: number of address bytes to send.
* @rdsr_dummy: dummy cycles needed for Read Status Register command
* in octal DTR mode.
* @rdsr_addr_nbytes: dummy address bytes needed for Read Status Register
@@ -377,6 +378,7 @@ struct spi_nor_flash_parameter {
u64 size;
u32 writesize;
u32 page_size;
+ u8 addr_nbytes;
u8 rdsr_dummy;
u8 rdsr_addr_nbytes;
diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index 058ce218d2af..90d7c25f7281 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -178,19 +178,16 @@ static int spi_nor_read_raw(struct spi_nor *nor, u32 addr, size_t len, u8 *buf)
static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
size_t len, void *buf)
{
- u8 addr_nbytes;
int ret;
- addr_nbytes = nor->addr_nbytes;
-
nor->read_opcode = SPINOR_OP_RDSFDP;
nor->addr_nbytes = 3;
nor->read_dummy = 8;
ret = spi_nor_read_raw(nor, addr, len, buf);
- nor->addr_nbytes = addr_nbytes;
/* Restore setup to its uninitialized state. */
+ nor->addr_nbytes = 0;
nor->read_opcode = 0;
nor->read_dummy = 0;
@@ -461,11 +458,11 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) {
case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY:
case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4:
- nor->addr_nbytes = 3;
+ params->addr_nbytes = 3;
break;
case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY:
- nor->addr_nbytes = 4;
+ params->addr_nbytes = 4;
break;
default:
@@ -652,7 +649,7 @@ static u8 spi_nor_smpt_addr_nbytes(const struct spi_nor *nor, const u32 settings
return 4;
case SMPT_CMD_ADDRESS_LEN_USE_CURRENT:
default:
- return nor->addr_nbytes;
+ return nor->params->addr_nbytes;
}
}
@@ -689,7 +686,6 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
u32 addr;
int err;
u8 i;
- u8 addr_nbytes;
u8 read_data_mask, map_id;
/* Use a kmalloc'ed bounce buffer to guarantee it is DMA-able. */
@@ -697,8 +693,6 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
if (!buf)
return ERR_PTR(-ENOMEM);
- addr_nbytes = nor->addr_nbytes;
-
map_id = 0;
/* Determine if there are any optional Detection Command Descriptors */
for (i = 0; i < smpt_len; i += 2) {
@@ -753,8 +747,8 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
/* fall through */
out:
kfree(buf);
- nor->addr_nbytes = addr_nbytes;
/* Restore setup to its uninitialized state. */
+ nor->addr_nbytes = 0;
nor->read_dummy = 0;
nor->read_opcode = 0;
return ret;
@@ -1096,7 +1090,7 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
* Spansion memory. However this quirk is no longer needed with new
* SFDP compliant memories.
*/
- nor->addr_nbytes = 4;
+ params->addr_nbytes = 4;
nor->flags |= SNOR_F_4B_OPCODES | SNOR_F_HAS_4BAIT;
/* fall through */
--
2.25.1
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v15 4/8] mtd: spi-nor: Do not change nor->addr_nbytes at SFDP parsing time
2022-05-09 22:10 ` [PATCH v15 4/8] mtd: spi-nor: Do not change nor->addr_nbytes at SFDP parsing time tkuw584924
@ 2022-05-12 21:45 ` Michael Walle
2022-05-31 11:30 ` Pratyush Yadav
1 sibling, 0 replies; 42+ messages in thread
From: Michael Walle @ 2022-05-12 21:45 UTC (permalink / raw)
To: tkuw584924
Cc: linux-mtd, tudor.ambarus, miquel.raynal, richard, vigneshr,
p.yadav, Bacem.Daassi, Takahiro Kuwano
Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>
> At the SFDP parsing time we should not change members of struct
> spi_nor,
> but instead fill members of struct spi_nor_flash_parameters which could
> leter on be used by callers. The caller will then decide if SFDP params
> should be used and more importantly when they should be used. Clean the
> code flow and don't initialize nor->addr_nbytes at SFDP parsing time.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> Tested-By: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
Nice!
Reviewed-by: Michael Walle <michael@walle.cc>
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 4/8] mtd: spi-nor: Do not change nor->addr_nbytes at SFDP parsing time
2022-05-09 22:10 ` [PATCH v15 4/8] mtd: spi-nor: Do not change nor->addr_nbytes at SFDP parsing time tkuw584924
2022-05-12 21:45 ` Michael Walle
@ 2022-05-31 11:30 ` Pratyush Yadav
2022-07-21 14:32 ` Tudor.Ambarus
2022-07-21 15:02 ` Tudor.Ambarus
1 sibling, 2 replies; 42+ messages in thread
From: Pratyush Yadav @ 2022-05-31 11:30 UTC (permalink / raw)
To: tkuw584924
Cc: linux-mtd, tudor.ambarus, miquel.raynal, richard, vigneshr,
michael, Bacem.Daassi, Takahiro Kuwano
On 10/05/22 07:10AM, tkuw584924@gmail.com wrote:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>
> At the SFDP parsing time we should not change members of struct spi_nor,
> but instead fill members of struct spi_nor_flash_parameters which could
> leter on be used by callers. The caller will then decide if SFDP params
s/leter/later/
> should be used and more importantly when they should be used. Clean the
> code flow and don't initialize nor->addr_nbytes at SFDP parsing time.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> Tested-By: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
> drivers/mtd/spi-nor/core.c | 5 ++---
> drivers/mtd/spi-nor/core.h | 2 ++
> drivers/mtd/spi-nor/sfdp.c | 18 ++++++------------
> 3 files changed, 10 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 7db6b41d7c30..dd71deba9f11 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2272,8 +2272,8 @@ static int spi_nor_default_setup(struct spi_nor *nor,
>
> static int spi_nor_set_addr_nbytes(struct spi_nor *nor)
> {
> - if (nor->addr_nbytes) {
> - /* already configured from SFDP */
> + if (nor->params->addr_nbytes) {
> + nor->addr_nbytes = nor->params->addr_nbytes;
> } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
> /*
> * In 8D-8D-8D mode, one byte takes half a cycle to transfer. So
> @@ -2518,7 +2518,6 @@ static void spi_nor_sfdp_init_params_deprecated(struct spi_nor *nor)
>
> if (spi_nor_parse_sfdp(nor)) {
> memcpy(nor->params, &sfdp_params, sizeof(*nor->params));
> - nor->addr_nbytes = 0;
> nor->flags &= ~SNOR_F_4B_OPCODES;
> }
> }
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index fe7683fe1b4d..41df8bc8e008 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -345,6 +345,7 @@ struct spi_nor_otp {
> * @writesize Minimal writable flash unit size. Defaults to 1. Set to
> * ECC unit size for ECC-ed flashes.
> * @page_size: the page size of the SPI NOR flash memory.
> + * @addr_nbytes: number of address bytes to send.
> * @rdsr_dummy: dummy cycles needed for Read Status Register command
> * in octal DTR mode.
> * @rdsr_addr_nbytes: dummy address bytes needed for Read Status Register
> @@ -377,6 +378,7 @@ struct spi_nor_flash_parameter {
> u64 size;
> u32 writesize;
> u32 page_size;
> + u8 addr_nbytes;
> u8 rdsr_dummy;
> u8 rdsr_addr_nbytes;
>
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index 058ce218d2af..90d7c25f7281 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -178,19 +178,16 @@ static int spi_nor_read_raw(struct spi_nor *nor, u32 addr, size_t len, u8 *buf)
> static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
> size_t len, void *buf)
> {
> - u8 addr_nbytes;
> int ret;
>
> - addr_nbytes = nor->addr_nbytes;
> -
> nor->read_opcode = SPINOR_OP_RDSFDP;
> nor->addr_nbytes = 3;
> nor->read_dummy = 8;
>
> ret = spi_nor_read_raw(nor, addr, len, buf);
>
> - nor->addr_nbytes = addr_nbytes;
> /* Restore setup to its uninitialized state. */
> + nor->addr_nbytes = 0;
> nor->read_opcode = 0;
> nor->read_dummy = 0;
>
> @@ -461,11 +458,11 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
> switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) {
> case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY:
> case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4:
> - nor->addr_nbytes = 3;
> + params->addr_nbytes = 3;
> break;
>
> case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY:
> - nor->addr_nbytes = 4;
> + params->addr_nbytes = 4;
> break;
>
> default:
> @@ -652,7 +649,7 @@ static u8 spi_nor_smpt_addr_nbytes(const struct spi_nor *nor, const u32 settings
> return 4;
> case SMPT_CMD_ADDRESS_LEN_USE_CURRENT:
> default:
> - return nor->addr_nbytes;
> + return nor->params->addr_nbytes;
> }
> }
>
> @@ -689,7 +686,6 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
> u32 addr;
> int err;
> u8 i;
> - u8 addr_nbytes;
> u8 read_data_mask, map_id;
>
> /* Use a kmalloc'ed bounce buffer to guarantee it is DMA-able. */
> @@ -697,8 +693,6 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
> if (!buf)
> return ERR_PTR(-ENOMEM);
>
> - addr_nbytes = nor->addr_nbytes;
> -
> map_id = 0;
> /* Determine if there are any optional Detection Command Descriptors */
> for (i = 0; i < smpt_len; i += 2) {
> @@ -753,8 +747,8 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
> /* fall through */
> out:
> kfree(buf);
> - nor->addr_nbytes = addr_nbytes;
> /* Restore setup to its uninitialized state. */
> + nor->addr_nbytes = 0;
Same as before, I don't think this function should know or care about
the default or uninitialised values.
> nor->read_dummy = 0;
> nor->read_opcode = 0;
> return ret;
> @@ -1096,7 +1090,7 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
> * Spansion memory. However this quirk is no longer needed with new
> * SFDP compliant memories.
> */
> - nor->addr_nbytes = 4;
> + params->addr_nbytes = 4;
Patch looks good at first glance but I have not looked very carefully if
it might cause some small issues.
> nor->flags |= SNOR_F_4B_OPCODES | SNOR_F_HAS_4BAIT;
>
> /* fall through */
> --
> 2.25.1
>
issi.c's is25lp256_post_bfpt_fixups() also sets nor->addr_nbytes. Should
that be updated to use params->addr_nbytes instead?
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v15 4/8] mtd: spi-nor: Do not change nor->addr_nbytes at SFDP parsing time
2022-05-31 11:30 ` Pratyush Yadav
@ 2022-07-21 14:32 ` Tudor.Ambarus
2022-07-21 15:02 ` Tudor.Ambarus
1 sibling, 0 replies; 42+ messages in thread
From: Tudor.Ambarus @ 2022-07-21 14:32 UTC (permalink / raw)
To: p.yadav, tkuw584924
Cc: linux-mtd, miquel.raynal, richard, vigneshr, michael,
Bacem.Daassi, Takahiro.Kuwano
On 5/31/22 14:30, Pratyush Yadav wrote:
> issi.c's is25lp256_post_bfpt_fixups() also sets nor->addr_nbytes. Should
> that be updated to use params->addr_nbytes instead?
Yes, it should, good catch! I've revisited this patch, otherwise
it looks good.
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 4/8] mtd: spi-nor: Do not change nor->addr_nbytes at SFDP parsing time
2022-05-31 11:30 ` Pratyush Yadav
2022-07-21 14:32 ` Tudor.Ambarus
@ 2022-07-21 15:02 ` Tudor.Ambarus
1 sibling, 0 replies; 42+ messages in thread
From: Tudor.Ambarus @ 2022-07-21 15:02 UTC (permalink / raw)
To: p.yadav, tkuw584924
Cc: linux-mtd, miquel.raynal, richard, vigneshr, michael,
Bacem.Daassi, Takahiro.Kuwano
On 5/31/22 14:30, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 10/05/22 07:10AM, tkuw584924@gmail.com wrote:
>> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>>
>> At the SFDP parsing time we should not change members of struct spi_nor,
>> but instead fill members of struct spi_nor_flash_parameters which could
>> leter on be used by callers. The caller will then decide if SFDP params
>
> s/leter/later/
>
>> should be used and more importantly when they should be used. Clean the
>> code flow and don't initialize nor->addr_nbytes at SFDP parsing time.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> Tested-By: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> ---
>> drivers/mtd/spi-nor/core.c | 5 ++---
>> drivers/mtd/spi-nor/core.h | 2 ++
>> drivers/mtd/spi-nor/sfdp.c | 18 ++++++------------
>> 3 files changed, 10 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 7db6b41d7c30..dd71deba9f11 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -2272,8 +2272,8 @@ static int spi_nor_default_setup(struct spi_nor *nor,
>>
>> static int spi_nor_set_addr_nbytes(struct spi_nor *nor)
>> {
>> - if (nor->addr_nbytes) {
>> - /* already configured from SFDP */
>> + if (nor->params->addr_nbytes) {
>> + nor->addr_nbytes = nor->params->addr_nbytes;
>> } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
>> /*
>> * In 8D-8D-8D mode, one byte takes half a cycle to transfer. So
>> @@ -2518,7 +2518,6 @@ static void spi_nor_sfdp_init_params_deprecated(struct spi_nor *nor)
>>
>> if (spi_nor_parse_sfdp(nor)) {
>> memcpy(nor->params, &sfdp_params, sizeof(*nor->params));
>> - nor->addr_nbytes = 0;
>> nor->flags &= ~SNOR_F_4B_OPCODES;
>> }
>> }
>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>> index fe7683fe1b4d..41df8bc8e008 100644
>> --- a/drivers/mtd/spi-nor/core.h
>> +++ b/drivers/mtd/spi-nor/core.h
>> @@ -345,6 +345,7 @@ struct spi_nor_otp {
>> * @writesize Minimal writable flash unit size. Defaults to 1. Set to
>> * ECC unit size for ECC-ed flashes.
>> * @page_size: the page size of the SPI NOR flash memory.
>> + * @addr_nbytes: number of address bytes to send.
>> * @rdsr_dummy: dummy cycles needed for Read Status Register command
>> * in octal DTR mode.
>> * @rdsr_addr_nbytes: dummy address bytes needed for Read Status Register
>> @@ -377,6 +378,7 @@ struct spi_nor_flash_parameter {
>> u64 size;
>> u32 writesize;
>> u32 page_size;
>> + u8 addr_nbytes;
>> u8 rdsr_dummy;
>> u8 rdsr_addr_nbytes;
>>
>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>> index 058ce218d2af..90d7c25f7281 100644
>> --- a/drivers/mtd/spi-nor/sfdp.c
>> +++ b/drivers/mtd/spi-nor/sfdp.c
>> @@ -178,19 +178,16 @@ static int spi_nor_read_raw(struct spi_nor *nor, u32 addr, size_t len, u8 *buf)
>> static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
>> size_t len, void *buf)
>> {
>> - u8 addr_nbytes;
>> int ret;
>>
>> - addr_nbytes = nor->addr_nbytes;
>> -
>> nor->read_opcode = SPINOR_OP_RDSFDP;
>> nor->addr_nbytes = 3;
>> nor->read_dummy = 8;
>>
>> ret = spi_nor_read_raw(nor, addr, len, buf);
>>
>> - nor->addr_nbytes = addr_nbytes;
>> /* Restore setup to its uninitialized state. */
>> + nor->addr_nbytes = 0;
>> nor->read_opcode = 0;
>> nor->read_dummy = 0;
>>
>> @@ -461,11 +458,11 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>> switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) {
>> case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY:
>> case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4:
>> - nor->addr_nbytes = 3;
>> + params->addr_nbytes = 3;
>> break;
>>
>> case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY:
>> - nor->addr_nbytes = 4;
>> + params->addr_nbytes = 4;
>> break;
>>
>> default:
>> @@ -652,7 +649,7 @@ static u8 spi_nor_smpt_addr_nbytes(const struct spi_nor *nor, const u32 settings
>> return 4;
>> case SMPT_CMD_ADDRESS_LEN_USE_CURRENT:
>> default:
>> - return nor->addr_nbytes;
>> + return nor->params->addr_nbytes;
>> }
>> }
>>
>> @@ -689,7 +686,6 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>> u32 addr;
>> int err;
>> u8 i;
>> - u8 addr_nbytes;
>> u8 read_data_mask, map_id;
>>
>> /* Use a kmalloc'ed bounce buffer to guarantee it is DMA-able. */
>> @@ -697,8 +693,6 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>> if (!buf)
>> return ERR_PTR(-ENOMEM);
>>
>> - addr_nbytes = nor->addr_nbytes;
>> -
>> map_id = 0;
>> /* Determine if there are any optional Detection Command Descriptors */
>> for (i = 0; i < smpt_len; i += 2) {
>> @@ -753,8 +747,8 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>> /* fall through */
>> out:
>> kfree(buf);
>> - nor->addr_nbytes = addr_nbytes;
>> /* Restore setup to its uninitialized state. */
>> + nor->addr_nbytes = 0;
>
> Same as before, I don't think this function should know or care about
> the default or uninitialised values.
>
well, maybe. nor->addr_nbytes comes with value zero when this method is called.
We gratuitously save it's zero value in a local variable and then we restore
it. The restore of the value could signify to the reader that nor->addr_nbytes
is already initialized with a sane value, which is not the case, so I find the
code more readable in my version. But I don't mind too much, so I'll drop this
particular change.
Thanks,
ta
>> nor->read_dummy = 0;
>> nor->read_opcode = 0;
>> return ret;
>> @@ -1096,7 +1090,7 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
>> * Spansion memory. However this quirk is no longer needed with new
>> * SFDP compliant memories.
>> */
>> - nor->addr_nbytes = 4;
>> + params->addr_nbytes = 4;
>
> Patch looks good at first glance but I have not looked very carefully if
> it might cause some small issues.
>
>> nor->flags |= SNOR_F_4B_OPCODES | SNOR_F_HAS_4BAIT;
>>
>> /* fall through */
>> --
>> 2.25.1
>>
>
> issi.c's is25lp256_post_bfpt_fixups() also sets nor->addr_nbytes. Should
> that be updated to use params->addr_nbytes instead?
>
> --
> Regards,
> Pratyush Yadav
> Texas Instruments Inc.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v15 5/8] mtd: spi-nor: core: Couple the number of address bytes with the address mode
2022-05-09 22:10 [PATCH v15 0/8] mtd: spi-nor: Add support for Infineon s25hl-t/s25hs-t tkuw584924
` (3 preceding siblings ...)
2022-05-09 22:10 ` [PATCH v15 4/8] mtd: spi-nor: Do not change nor->addr_nbytes at SFDP parsing time tkuw584924
@ 2022-05-09 22:10 ` tkuw584924
2022-05-12 22:07 ` Michael Walle
2022-05-09 22:10 ` [PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse tkuw584924
` (2 subsequent siblings)
7 siblings, 1 reply; 42+ messages in thread
From: tkuw584924 @ 2022-05-09 22:10 UTC (permalink / raw)
To: linux-mtd
Cc: tudor.ambarus, miquel.raynal, richard, vigneshr, p.yadav,
michael, tkuw584924, Bacem.Daassi, Takahiro Kuwano
From: Tudor Ambarus <tudor.ambarus@microchip.com>
Some of Infineon chips support volatile version of configuration registers
and it is recommended to update volatile registers in the field application
due to a risk of the non-volatile registers corruption by power interrupt.
Such a volatile configuration register is used to enable the Quad mode.
The register write sequence requires the number of bytes of address in
order to be programmed. As it was before, the nor->addr_nbytes was set to 4
before calling the volatile Quad enable method. This was incorrect because
the Write Any Register command does not have a 4B opcode equivalent and the
address mode was still at default (3-byte mode) and not changed to 4 by
entering in the 4 Byte Address Mode, so the operation failed.
Move the setting of the number of bytes of address after the Quad Enable
method to allow reads or writes to registers that require the number of
address bytes to work with the default address mode. The number of address
bytes and the address mode are tightly coupled, this is a natural change.
Other (standard) Quad Enable methods are not affected, as they don't
require the number of address bytes, so no functionality changes expected.
Reported-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
Tested-By: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
drivers/mtd/spi-nor/core.c | 134 +++++++++++++++++++------------------
1 file changed, 70 insertions(+), 64 deletions(-)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index dd71deba9f11..1c14a95a23fd 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2270,49 +2270,6 @@ static int spi_nor_default_setup(struct spi_nor *nor,
return 0;
}
-static int spi_nor_set_addr_nbytes(struct spi_nor *nor)
-{
- if (nor->params->addr_nbytes) {
- nor->addr_nbytes = nor->params->addr_nbytes;
- } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
- /*
- * In 8D-8D-8D mode, one byte takes half a cycle to transfer. So
- * in this protocol an odd address width cannot be used because
- * then the address phase would only span a cycle and a half.
- * Half a cycle would be left over. We would then have to start
- * the dummy phase in the middle of a cycle and so too the data
- * phase, and we will end the transaction with half a cycle left
- * over.
- *
- * Force all 8D-8D-8D flashes to use an address width of 4 to
- * avoid this situation.
- */
- nor->addr_nbytes = 4;
- } else if (nor->info->addr_nbytes) {
- nor->addr_nbytes = nor->info->addr_nbytes;
- } else {
- nor->addr_nbytes = 3;
- }
-
- if (nor->addr_nbytes == 3 && nor->params->size > 0x1000000) {
- /* enable 4-byte addressing if the device exceeds 16MiB */
- nor->addr_nbytes = 4;
- }
-
- if (nor->addr_nbytes > SPI_NOR_MAX_ADDR_NBYTES) {
- dev_dbg(nor->dev, "address width is too large: %u\n",
- nor->addr_nbytes);
- return -EINVAL;
- }
-
- /* Set 4byte opcodes when possible. */
- if (nor->addr_nbytes == 4 && nor->flags & SNOR_F_4B_OPCODES &&
- !(nor->flags & SNOR_F_HAS_4BAIT))
- spi_nor_set_4byte_opcodes(nor);
-
- return 0;
-}
-
static int spi_nor_setup(struct spi_nor *nor,
const struct spi_nor_hwcaps *hwcaps)
{
@@ -2322,10 +2279,7 @@ static int spi_nor_setup(struct spi_nor *nor,
ret = nor->params->setup(nor, hwcaps);
else
ret = spi_nor_default_setup(nor, hwcaps);
- if (ret)
- return ret;
-
- return spi_nor_set_addr_nbytes(nor);
+ return ret;
}
/**
@@ -2707,6 +2661,74 @@ static int spi_nor_quad_enable(struct spi_nor *nor)
return nor->params->quad_enable(nor);
}
+static int spi_nor_set_addr_nbytes(struct spi_nor *nor)
+{
+ if (nor->params->addr_nbytes) {
+ nor->addr_nbytes = nor->params->addr_nbytes;
+ } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
+ /*
+ * In 8D-8D-8D mode, one byte takes half a cycle to transfer. So
+ * in this protocol an odd address width cannot be used because
+ * then the address phase would only span a cycle and a half.
+ * Half a cycle would be left over. We would then have to start
+ * the dummy phase in the middle of a cycle and so too the data
+ * phase, and we will end the transaction with half a cycle left
+ * over.
+ *
+ * Force all 8D-8D-8D flashes to use an address width of 4 to
+ * avoid this situation.
+ */
+ nor->addr_nbytes = 4;
+ } else if (nor->info->addr_nbytes) {
+ nor->addr_nbytes = nor->info->addr_nbytes;
+ } else {
+ nor->addr_nbytes = 3;
+ }
+
+ if (nor->addr_nbytes == 3 && nor->params->size > 0x1000000) {
+ /* enable 4-byte addressing if the device exceeds 16MiB */
+ nor->addr_nbytes = 4;
+ }
+
+ if (nor->addr_nbytes > SPI_NOR_MAX_ADDR_NBYTES) {
+ dev_dbg(nor->dev, "address width is too large: %u\n",
+ nor->addr_nbytes);
+ return -EINVAL;
+ }
+
+ /* Set 4byte opcodes when possible. */
+ if (nor->addr_nbytes == 4 && nor->flags & SNOR_F_4B_OPCODES &&
+ !(nor->flags & SNOR_F_HAS_4BAIT))
+ spi_nor_set_4byte_opcodes(nor);
+
+ return 0;
+}
+
+static int spi_nor_set_addr_mode(struct spi_nor *nor)
+{
+ int ret;
+
+ ret = spi_nor_set_addr_nbytes(nor);
+ if (ret)
+ return ret;
+
+ if (nor->addr_nbytes == 4 && nor->read_proto != SNOR_PROTO_8_8_8_DTR &&
+ !(nor->flags & SNOR_F_4B_OPCODES)) {
+ /*
+ * If the RESET# pin isn't hooked up properly, or the system
+ * otherwise doesn't perform a reset command in the boot
+ * sequence, it's impossible to 100% protect against unexpected
+ * reboots (e.g., crashes). Warn the user (or hopefully, system
+ * designer) that this is bad.
+ */
+ WARN_ONCE(nor->flags & SNOR_F_BROKEN_RESET,
+ "enabling reset hack; may not recover from unexpected reboots\n");
+ nor->params->set_4byte_addr_mode(nor, true);
+ }
+
+ return 0;
+}
+
static int spi_nor_init(struct spi_nor *nor)
{
int err;
@@ -2738,22 +2760,7 @@ static int spi_nor_init(struct spi_nor *nor)
nor->flags & SNOR_F_SWP_IS_VOLATILE))
spi_nor_try_unlock_all(nor);
- if (nor->addr_nbytes == 4 &&
- nor->read_proto != SNOR_PROTO_8_8_8_DTR &&
- !(nor->flags & SNOR_F_4B_OPCODES)) {
- /*
- * If the RESET# pin isn't hooked up properly, or the system
- * otherwise doesn't perform a reset command in the boot
- * sequence, it's impossible to 100% protect against unexpected
- * reboots (e.g., crashes). Warn the user (or hopefully, system
- * designer) that this is bad.
- */
- WARN_ONCE(nor->flags & SNOR_F_BROKEN_RESET,
- "enabling reset hack; may not recover from unexpected reboots\n");
- nor->params->set_4byte_addr_mode(nor, true);
- }
-
- return 0;
+ return spi_nor_set_addr_mode(nor);
}
/**
@@ -3014,7 +3021,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
* - select op codes for (Fast) Read, Page Program and Sector Erase.
* - set the number of dummy cycles (mode cycles + wait states).
* - set the SPI protocols for register and memory accesses.
- * - set the address width.
*/
ret = spi_nor_setup(nor, hwcaps);
if (ret)
--
2.25.1
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v15 5/8] mtd: spi-nor: core: Couple the number of address bytes with the address mode
2022-05-09 22:10 ` [PATCH v15 5/8] mtd: spi-nor: core: Couple the number of address bytes with the address mode tkuw584924
@ 2022-05-12 22:07 ` Michael Walle
0 siblings, 0 replies; 42+ messages in thread
From: Michael Walle @ 2022-05-12 22:07 UTC (permalink / raw)
To: tkuw584924
Cc: linux-mtd, tudor.ambarus, miquel.raynal, richard, vigneshr,
p.yadav, Bacem.Daassi, Takahiro Kuwano
Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>
> Some of Infineon chips support volatile version of configuration
> registers
> and it is recommended to update volatile registers in the field
> application
> due to a risk of the non-volatile registers corruption by power
> interrupt.
> Such a volatile configuration register is used to enable the Quad mode.
> The register write sequence requires the number of bytes of address in
> order to be programmed. As it was before, the nor->addr_nbytes was set
> to 4
> before calling the volatile Quad enable method. This was incorrect
> because
> the Write Any Register command does not have a 4B opcode equivalent and
> the
> address mode was still at default (3-byte mode) and not changed to 4 by
> entering in the 4 Byte Address Mode, so the operation failed.
>
> Move the setting of the number of bytes of address after the Quad
> Enable
> method to allow reads or writes to registers that require the number of
> address bytes to work with the default address mode. The number of
> address
> bytes and the address mode are tightly coupled, this is a natural
> change.
>
> Other (standard) Quad Enable methods are not affected, as they don't
> require the number of address bytes, so no functionality changes
> expected.
>
> Reported-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> Tested-By: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
> drivers/mtd/spi-nor/core.c | 134 +++++++++++++++++++------------------
> 1 file changed, 70 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index dd71deba9f11..1c14a95a23fd 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2270,49 +2270,6 @@ static int spi_nor_default_setup(struct spi_nor
> *nor,
> return 0;
> }
>
> -static int spi_nor_set_addr_nbytes(struct spi_nor *nor)
> -{
> - if (nor->params->addr_nbytes) {
> - nor->addr_nbytes = nor->params->addr_nbytes;
> - } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
> - /*
> - * In 8D-8D-8D mode, one byte takes half a cycle to transfer. So
> - * in this protocol an odd address width cannot be used because
> - * then the address phase would only span a cycle and a half.
> - * Half a cycle would be left over. We would then have to start
> - * the dummy phase in the middle of a cycle and so too the data
> - * phase, and we will end the transaction with half a cycle left
> - * over.
> - *
> - * Force all 8D-8D-8D flashes to use an address width of 4 to
> - * avoid this situation.
> - */
> - nor->addr_nbytes = 4;
> - } else if (nor->info->addr_nbytes) {
> - nor->addr_nbytes = nor->info->addr_nbytes;
> - } else {
> - nor->addr_nbytes = 3;
> - }
> -
> - if (nor->addr_nbytes == 3 && nor->params->size > 0x1000000) {
> - /* enable 4-byte addressing if the device exceeds 16MiB */
> - nor->addr_nbytes = 4;
> - }
> -
> - if (nor->addr_nbytes > SPI_NOR_MAX_ADDR_NBYTES) {
> - dev_dbg(nor->dev, "address width is too large: %u\n",
> - nor->addr_nbytes);
> - return -EINVAL;
> - }
> -
> - /* Set 4byte opcodes when possible. */
> - if (nor->addr_nbytes == 4 && nor->flags & SNOR_F_4B_OPCODES &&
> - !(nor->flags & SNOR_F_HAS_4BAIT))
> - spi_nor_set_4byte_opcodes(nor);
> -
> - return 0;
> -}
> -
> static int spi_nor_setup(struct spi_nor *nor,
> const struct spi_nor_hwcaps *hwcaps)
> {
> @@ -2322,10 +2279,7 @@ static int spi_nor_setup(struct spi_nor *nor,
> ret = nor->params->setup(nor, hwcaps);
> else
> ret = spi_nor_default_setup(nor, hwcaps);
> - if (ret)
> - return ret;
> -
> - return spi_nor_set_addr_nbytes(nor);
> + return ret;
> }
>
> /**
> @@ -2707,6 +2661,74 @@ static int spi_nor_quad_enable(struct spi_nor
> *nor)
> return nor->params->quad_enable(nor);
> }
>
> +static int spi_nor_set_addr_nbytes(struct spi_nor *nor)
> +{
> + if (nor->params->addr_nbytes) {
> + nor->addr_nbytes = nor->params->addr_nbytes;
> + } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
> + /*
> + * In 8D-8D-8D mode, one byte takes half a cycle to transfer. So
> + * in this protocol an odd address width cannot be used because
> + * then the address phase would only span a cycle and a half.
> + * Half a cycle would be left over. We would then have to start
> + * the dummy phase in the middle of a cycle and so too the data
> + * phase, and we will end the transaction with half a cycle left
> + * over.
> + *
> + * Force all 8D-8D-8D flashes to use an address width of 4 to
> + * avoid this situation.
> + */
> + nor->addr_nbytes = 4;
> + } else if (nor->info->addr_nbytes) {
> + nor->addr_nbytes = nor->info->addr_nbytes;
> + } else {
> + nor->addr_nbytes = 3;
> + }
> +
> + if (nor->addr_nbytes == 3 && nor->params->size > 0x1000000) {
> + /* enable 4-byte addressing if the device exceeds 16MiB */
> + nor->addr_nbytes = 4;
> + }
> +
> + if (nor->addr_nbytes > SPI_NOR_MAX_ADDR_NBYTES) {
> + dev_dbg(nor->dev, "address width is too large: %u\n",
> + nor->addr_nbytes);
> + return -EINVAL;
> + }
> +
> + /* Set 4byte opcodes when possible. */
> + if (nor->addr_nbytes == 4 && nor->flags & SNOR_F_4B_OPCODES &&
> + !(nor->flags & SNOR_F_HAS_4BAIT))
> + spi_nor_set_4byte_opcodes(nor);
> +
> + return 0;
> +}
> +
> +static int spi_nor_set_addr_mode(struct spi_nor *nor)
> +{
> + int ret;
> +
> + ret = spi_nor_set_addr_nbytes(nor);
> + if (ret)
> + return ret;
> +
> + if (nor->addr_nbytes == 4 && nor->read_proto != SNOR_PROTO_8_8_8_DTR
> &&
> + !(nor->flags & SNOR_F_4B_OPCODES)) {
> + /*
> + * If the RESET# pin isn't hooked up properly, or the system
> + * otherwise doesn't perform a reset command in the boot
> + * sequence, it's impossible to 100% protect against unexpected
> + * reboots (e.g., crashes). Warn the user (or hopefully, system
> + * designer) that this is bad.
> + */
> + WARN_ONCE(nor->flags & SNOR_F_BROKEN_RESET,
> + "enabling reset hack; may not recover from unexpected
> reboots\n");
> + nor->params->set_4byte_addr_mode(nor, true);
Why don't we check the return code here? I know it was this
way before, but that looks wrong.
-michael
> + }
> +
> + return 0;
> +}
> +
> static int spi_nor_init(struct spi_nor *nor)
> {
> int err;
> @@ -2738,22 +2760,7 @@ static int spi_nor_init(struct spi_nor *nor)
> nor->flags & SNOR_F_SWP_IS_VOLATILE))
> spi_nor_try_unlock_all(nor);
>
> - if (nor->addr_nbytes == 4 &&
> - nor->read_proto != SNOR_PROTO_8_8_8_DTR &&
> - !(nor->flags & SNOR_F_4B_OPCODES)) {
> - /*
> - * If the RESET# pin isn't hooked up properly, or the system
> - * otherwise doesn't perform a reset command in the boot
> - * sequence, it's impossible to 100% protect against unexpected
> - * reboots (e.g., crashes). Warn the user (or hopefully, system
> - * designer) that this is bad.
> - */
> - WARN_ONCE(nor->flags & SNOR_F_BROKEN_RESET,
> - "enabling reset hack; may not recover from unexpected
> reboots\n");
> - nor->params->set_4byte_addr_mode(nor, true);
> - }
> -
> - return 0;
> + return spi_nor_set_addr_mode(nor);
> }
>
> /**
> @@ -3014,7 +3021,6 @@ int spi_nor_scan(struct spi_nor *nor, const char
> *name,
> * - select op codes for (Fast) Read, Page Program and Sector Erase.
> * - set the number of dummy cycles (mode cycles + wait states).
> * - set the SPI protocols for register and memory accesses.
> - * - set the address width.
> */
> ret = spi_nor_setup(nor, hwcaps);
> if (ret)
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
2022-05-09 22:10 [PATCH v15 0/8] mtd: spi-nor: Add support for Infineon s25hl-t/s25hs-t tkuw584924
` (4 preceding siblings ...)
2022-05-09 22:10 ` [PATCH v15 5/8] mtd: spi-nor: core: Couple the number of address bytes with the address mode tkuw584924
@ 2022-05-09 22:10 ` tkuw584924
2022-05-12 22:14 ` Michael Walle
2022-05-09 22:10 ` [PATCH v15 7/8] mtd: spi-nor: spansion: Add local function to discover page size tkuw584924
2022-05-09 22:10 ` [PATCH v15 8/8] mtd: spi-nor: spansion: Add s25hl-t/s25hs-t IDs and fixups tkuw584924
7 siblings, 1 reply; 42+ messages in thread
From: tkuw584924 @ 2022-05-09 22:10 UTC (permalink / raw)
To: linux-mtd
Cc: tudor.ambarus, miquel.raynal, richard, vigneshr, p.yadav,
michael, tkuw584924, Bacem.Daassi, Takahiro Kuwano
From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
In 4BAIT parse, keep nor->params->addr_width because it may be used as
current address mode in SMPT parse later on.
Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
drivers/mtd/spi-nor/core.c | 2 ++
drivers/mtd/spi-nor/sfdp.c | 1 -
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 1c14a95a23fd..051b26b95e2a 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2664,6 +2664,8 @@ static int spi_nor_quad_enable(struct spi_nor *nor)
static int spi_nor_set_addr_nbytes(struct spi_nor *nor)
{
if (nor->params->addr_nbytes) {
+ if (nor->flags & SNOR_F_HAS_4BAIT)
+ nor->params->addr_nbytes = 4;
nor->addr_nbytes = nor->params->addr_nbytes;
} else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
/*
diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index 90d7c25f7281..7240d9d74b8f 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -1090,7 +1090,6 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
* Spansion memory. However this quirk is no longer needed with new
* SFDP compliant memories.
*/
- params->addr_nbytes = 4;
nor->flags |= SNOR_F_4B_OPCODES | SNOR_F_HAS_4BAIT;
/* fall through */
--
2.25.1
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
2022-05-09 22:10 ` [PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse tkuw584924
@ 2022-05-12 22:14 ` Michael Walle
2022-05-13 1:26 ` Takahiro Kuwano
0 siblings, 1 reply; 42+ messages in thread
From: Michael Walle @ 2022-05-12 22:14 UTC (permalink / raw)
To: tkuw584924
Cc: linux-mtd, tudor.ambarus, miquel.raynal, richard, vigneshr,
p.yadav, Bacem.Daassi, Takahiro Kuwano
Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>
> In 4BAIT parse, keep nor->params->addr_width because it may be used as
> current address mode in SMPT parse later on.
Mh I'm not sure this is needed at all.
SFDP spec says
Variable address length (the current setting of the address
length mode defines the address length)
and
When the length is defined as variable, the software or hardware
controlling the memory is aware of the address length mode last
set in the memory device and this same length of address.
We don't set any address mode until all the SFDP parsing is
over. Therefore we should always be in 3 byte mode, no?
-michael
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
2022-05-12 22:14 ` Michael Walle
@ 2022-05-13 1:26 ` Takahiro Kuwano
2022-05-13 9:40 ` Michael Walle
0 siblings, 1 reply; 42+ messages in thread
From: Takahiro Kuwano @ 2022-05-13 1:26 UTC (permalink / raw)
To: Michael Walle
Cc: linux-mtd, tudor.ambarus, miquel.raynal, richard, vigneshr,
p.yadav, Bacem.Daassi, Takahiro Kuwano
On 5/13/2022 7:14 AM, Michael Walle wrote:
> Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>
>> In 4BAIT parse, keep nor->params->addr_width because it may be used as
>> current address mode in SMPT parse later on.
>
> Mh I'm not sure this is needed at all.
>
> SFDP spec says
> Variable address length (the current setting of the address
> length mode defines the address length)
>
> and
> When the length is defined as variable, the software or hardware
> controlling the memory is aware of the address length mode last
> set in the memory device and this same length of address.
>
> We don't set any address mode until all the SFDP parsing is
> over. Therefore we should always be in 3 byte mode, no?
>
Actually there are some devices that have variable address length but
4 byte mode by default (I will work on those devices after this series
is settled). To support such case, I prefer to use params->addr_nbytes
as current address mode so that I can fix it in post_bfpt_fixup() hook.
Thanks,
Takahiro
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
2022-05-13 1:26 ` Takahiro Kuwano
@ 2022-05-13 9:40 ` Michael Walle
2022-05-14 3:51 ` Takahiro Kuwano
0 siblings, 1 reply; 42+ messages in thread
From: Michael Walle @ 2022-05-13 9:40 UTC (permalink / raw)
To: Takahiro Kuwano
Cc: linux-mtd, tudor.ambarus, miquel.raynal, richard, vigneshr,
p.yadav, Bacem.Daassi, Takahiro Kuwano
[btw the subject still has the old name of the addr_width]
Am 2022-05-13 03:26, schrieb Takahiro Kuwano:
> On 5/13/2022 7:14 AM, Michael Walle wrote:
>> Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>
>>> In 4BAIT parse, keep nor->params->addr_width because it may be used
>>> as
>>> current address mode in SMPT parse later on.
>>
>> Mh I'm not sure this is needed at all.
>>
>> SFDP spec says
>> Variable address length (the current setting of the address
>> length mode defines the address length)
>>
>> and
>> When the length is defined as variable, the software or hardware
>> controlling the memory is aware of the address length mode last
>> set in the memory device and this same length of address.
>>
>> We don't set any address mode until all the SFDP parsing is
>> over. Therefore we should always be in 3 byte mode, no?
>>
> Actually there are some devices that have variable address length but
> 4 byte mode by default (I will work on those devices after this series
> is settled). To support such case, I prefer to use params->addr_nbytes
> as current address mode so that I can fix it in post_bfpt_fixup() hook.
Are there public datasheets available? So these devices have a 3 byte
and a 4 byte mode, but after reset, they are in the 4 byte mode? Looks
like it should be fixed in a different way. I'm not sure the "current
mode" handling is correct.
We need to differentiate between the mode the flash currently is using
(nor->addr_nbytes) and the mode parsed by SFDP (params->addr_nbytes).
At some point, the mode is switched and nor->addr_nbytes becomes
params->addr_nbytes. It seems in your case nor->addr_nbytes should
be 4 right from the beginning. Which also means nor->addr_nbytes
should be 3 for the other cases (and probably not 0).
-michael
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
2022-05-13 9:40 ` Michael Walle
@ 2022-05-14 3:51 ` Takahiro Kuwano
2022-05-18 6:04 ` Takahiro Kuwano
` (2 more replies)
0 siblings, 3 replies; 42+ messages in thread
From: Takahiro Kuwano @ 2022-05-14 3:51 UTC (permalink / raw)
To: Michael Walle
Cc: linux-mtd, tudor.ambarus, miquel.raynal, richard, vigneshr,
p.yadav, Bacem.Daassi, Takahiro Kuwano
On 5/13/2022 6:40 PM, Michael Walle wrote:
> [btw the subject still has the old name of the addr_width]
>
Yes, it must be fixed in next rev.
> Am 2022-05-13 03:26, schrieb Takahiro Kuwano:
>> On 5/13/2022 7:14 AM, Michael Walle wrote:
>>> Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>
>>>> In 4BAIT parse, keep nor->params->addr_width because it may be used as
>>>> current address mode in SMPT parse later on.
>>>
>>> Mh I'm not sure this is needed at all.
>>>
>>> SFDP spec says
>>> Variable address length (the current setting of the address
>>> length mode defines the address length)
>>>
>>> and
>>> When the length is defined as variable, the software or hardware
>>> controlling the memory is aware of the address length mode last
>>> set in the memory device and this same length of address.
>>>
>>> We don't set any address mode until all the SFDP parsing is
>>> over. Therefore we should always be in 3 byte mode, no?
>>>
>> Actually there are some devices that have variable address length but
>> 4 byte mode by default (I will work on those devices after this series
>> is settled). To support such case, I prefer to use params->addr_nbytes
>> as current address mode so that I can fix it in post_bfpt_fixup() hook.
>
> Are there public datasheets available? So these devices have a 3 byte
I will send datasheets to you in another email. At this point, only
summary datasheet is available in website.
> and a 4 byte mode, but after reset, they are in the 4 byte mode? Looks
Yes.
> like it should be fixed in a different way. I'm not sure the "current
> mode" handling is correct.
>
Yes, we may want to introduce a new flag like SPI_NOR_4BAM_DEFAULT and check
the flag in BFPT parse. Once I send another series, please review.
> We need to differentiate between the mode the flash currently is using
> (nor->addr_nbytes) and the mode parsed by SFDP (params->addr_nbytes).
>
The flash's address mode affects the address length of Non-4B opcodes,
including read/write any register ops used in SMPT parse and Infineon
(spansion) specific hooks.
The 4B opcodes always take address length of 4 regardless of flash's
address mode. In these Infineon chips, 4B opcodes for read/program/
erase are available and 4BAIT advertises them. We don't have to enter
4 byte address mode for read/program/erase.
So, I think we need to differentiate between address length for
read/program/erase and flash's default address mode.
Obviously we are using nor->addr_nbytes as address length for read/
program/erase and should keep this usage.
For flash's default address mode, my preference is to use
params->addr_nbytes, but I should rename it to something like
params->def_addr_nbytes and rework spi_nor_set_addr_nbytes().
static int spi_nor_set_addr_nbytes(struct spi_nor *nor)
{
if (nor->flags & SNOR_F_HAS_4BAIT) {
nor->addr_nbytes = 4;
} else if (nor->params->def_addr_nbytes) {
nor->addr_nbytes = nor->params->def_addr_nbytes;
> At some point, the mode is switched and nor->addr_nbytes becomes
> params->addr_nbytes. It seems in your case nor->addr_nbytes should
> be 4 right from the beginning. Which also means nor->addr_nbytes
> should be 3 for the other cases (and probably not 0).
>
With param->def_addr_nbytes, I think we can keep nor->addr_nbytes = 0
during SFDP parse.
Thanks,
Takahiro
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
2022-05-14 3:51 ` Takahiro Kuwano
@ 2022-05-18 6:04 ` Takahiro Kuwano
2022-05-18 8:35 ` Michael Walle
2022-05-23 7:49 ` Michael Walle
2022-06-08 11:39 ` Tudor.Ambarus
2 siblings, 1 reply; 42+ messages in thread
From: Takahiro Kuwano @ 2022-05-18 6:04 UTC (permalink / raw)
To: Michael Walle, tudor.ambarus, p.yadav
Cc: linux-mtd, miquel.raynal, richard, vigneshr, Bacem.Daassi,
Takahiro Kuwano
Hello,
Any comments on how to handle this?
On 5/14/2022 12:51 PM, Takahiro Kuwano wrote:
> On 5/13/2022 6:40 PM, Michael Walle wrote:
>> [btw the subject still has the old name of the addr_width]
>>
> Yes, it must be fixed in next rev.
>
>> Am 2022-05-13 03:26, schrieb Takahiro Kuwano:
>>> On 5/13/2022 7:14 AM, Michael Walle wrote:
>>>> Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
>>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>>
>>>>> In 4BAIT parse, keep nor->params->addr_width because it may be used as
>>>>> current address mode in SMPT parse later on.
>>>>
>>>> Mh I'm not sure this is needed at all.
>>>>
>>>> SFDP spec says
>>>> Variable address length (the current setting of the address
>>>> length mode defines the address length)
>>>>
>>>> and
>>>> When the length is defined as variable, the software or hardware
>>>> controlling the memory is aware of the address length mode last
>>>> set in the memory device and this same length of address.
>>>>
>>>> We don't set any address mode until all the SFDP parsing is
>>>> over. Therefore we should always be in 3 byte mode, no?
>>>>
>>> Actually there are some devices that have variable address length but
>>> 4 byte mode by default (I will work on those devices after this series
>>> is settled). To support such case, I prefer to use params->addr_nbytes
>>> as current address mode so that I can fix it in post_bfpt_fixup() hook.
>>
>> Are there public datasheets available? So these devices have a 3 byte
> I will send datasheets to you in another email. At this point, only
> summary datasheet is available in website.
>
>> and a 4 byte mode, but after reset, they are in the 4 byte mode? Looks
> Yes.
>
>> like it should be fixed in a different way. I'm not sure the "current
>> mode" handling is correct.
>>
> Yes, we may want to introduce a new flag like SPI_NOR_4BAM_DEFAULT and check
> the flag in BFPT parse. Once I send another series, please review.
>
>> We need to differentiate between the mode the flash currently is using
>> (nor->addr_nbytes) and the mode parsed by SFDP (params->addr_nbytes).
>>
> The flash's address mode affects the address length of Non-4B opcodes,
> including read/write any register ops used in SMPT parse and Infineon
> (spansion) specific hooks.
>
> The 4B opcodes always take address length of 4 regardless of flash's
> address mode. In these Infineon chips, 4B opcodes for read/program/
> erase are available and 4BAIT advertises them. We don't have to enter
> 4 byte address mode for read/program/erase.
>
> So, I think we need to differentiate between address length for
> read/program/erase and flash's default address mode.
>
> Obviously we are using nor->addr_nbytes as address length for read/
> program/erase and should keep this usage.
>
> For flash's default address mode, my preference is to use
> params->addr_nbytes, but I should rename it to something like
> params->def_addr_nbytes and rework spi_nor_set_addr_nbytes().
>
> static int spi_nor_set_addr_nbytes(struct spi_nor *nor)
> {
> if (nor->flags & SNOR_F_HAS_4BAIT) {
> nor->addr_nbytes = 4;
> } else if (nor->params->def_addr_nbytes) {
> nor->addr_nbytes = nor->params->def_addr_nbytes;
>
>> At some point, the mode is switched and nor->addr_nbytes becomes
>> params->addr_nbytes. It seems in your case nor->addr_nbytes should
>> be 4 right from the beginning. Which also means nor->addr_nbytes
>> should be 3 for the other cases (and probably not 0).
>>
> With param->def_addr_nbytes, I think we can keep nor->addr_nbytes = 0
> during SFDP parse.
>
> Thanks,
> Takahiro
>
>
>
>
>
>
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
2022-05-18 6:04 ` Takahiro Kuwano
@ 2022-05-18 8:35 ` Michael Walle
2022-05-18 9:12 ` Takahiro Kuwano
0 siblings, 1 reply; 42+ messages in thread
From: Michael Walle @ 2022-05-18 8:35 UTC (permalink / raw)
To: Takahiro Kuwano
Cc: tudor.ambarus, p.yadav, linux-mtd, miquel.raynal, richard,
vigneshr, Bacem.Daassi, Takahiro Kuwano
Hi,
Am 2022-05-18 08:04, schrieb Takahiro Kuwano:
> Any comments on how to handle this?
It's not even been a week since the former mail. Please give us some
time :)
-michael
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
2022-05-18 8:35 ` Michael Walle
@ 2022-05-18 9:12 ` Takahiro Kuwano
0 siblings, 0 replies; 42+ messages in thread
From: Takahiro Kuwano @ 2022-05-18 9:12 UTC (permalink / raw)
To: Michael Walle
Cc: tudor.ambarus, p.yadav, linux-mtd, miquel.raynal, richard,
vigneshr, Bacem.Daassi, Takahiro Kuwano
On 5/18/2022 5:35 PM, Michael Walle wrote:
> Hi,
>
> Am 2022-05-18 08:04, schrieb Takahiro Kuwano:
>> Any comments on how to handle this?
>
> It's not even been a week since the former mail. Please give us some
> time :)
>
Sorry, I need to be more patient.
Thanks,
Takahiro
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
2022-05-14 3:51 ` Takahiro Kuwano
2022-05-18 6:04 ` Takahiro Kuwano
@ 2022-05-23 7:49 ` Michael Walle
2022-05-23 9:56 ` Takahiro Kuwano
2022-07-21 16:06 ` Tudor.Ambarus
2022-06-08 11:39 ` Tudor.Ambarus
2 siblings, 2 replies; 42+ messages in thread
From: Michael Walle @ 2022-05-23 7:49 UTC (permalink / raw)
To: Takahiro Kuwano
Cc: linux-mtd, tudor.ambarus, miquel.raynal, richard, vigneshr,
p.yadav, Bacem.Daassi, Takahiro Kuwano
Am 2022-05-14 05:51, schrieb Takahiro Kuwano:
> On 5/13/2022 6:40 PM, Michael Walle wrote:
>> [btw the subject still has the old name of the addr_width]
>>
> Yes, it must be fixed in next rev.
>
>> Am 2022-05-13 03:26, schrieb Takahiro Kuwano:
>>> On 5/13/2022 7:14 AM, Michael Walle wrote:
>>>> Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
>>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>>
>>>>> In 4BAIT parse, keep nor->params->addr_width because it may be used
>>>>> as
>>>>> current address mode in SMPT parse later on.
>>>>
>>>> Mh I'm not sure this is needed at all.
>>>>
>>>> SFDP spec says
>>>> Variable address length (the current setting of the address
>>>> length mode defines the address length)
>>>>
>>>> and
>>>> When the length is defined as variable, the software or hardware
>>>> controlling the memory is aware of the address length mode last
>>>> set in the memory device and this same length of address.
>>>>
>>>> We don't set any address mode until all the SFDP parsing is
>>>> over. Therefore we should always be in 3 byte mode, no?
>>>>
>>> Actually there are some devices that have variable address length but
>>> 4 byte mode by default (I will work on those devices after this
>>> series
>>> is settled). To support such case, I prefer to use
>>> params->addr_nbytes
>>> as current address mode so that I can fix it in post_bfpt_fixup()
>>> hook.
>>
>> Are there public datasheets available? So these devices have a 3 byte
> I will send datasheets to you in another email. At this point, only
> summary datasheet is available in website.
>
>> and a 4 byte mode, but after reset, they are in the 4 byte mode? Looks
> Yes.
>
>> like it should be fixed in a different way. I'm not sure the "current
>> mode" handling is correct.
>>
> Yes, we may want to introduce a new flag like SPI_NOR_4BAM_DEFAULT and
> check
> the flag in BFPT parse. Once I send another series, please review.
>
>> We need to differentiate between the mode the flash currently is using
>> (nor->addr_nbytes) and the mode parsed by SFDP (params->addr_nbytes).
>>
> The flash's address mode affects the address length of Non-4B opcodes,
> including read/write any register ops used in SMPT parse and Infineon
> (spansion) specific hooks.
>
> The 4B opcodes always take address length of 4 regardless of flash's
> address mode. In these Infineon chips, 4B opcodes for read/program/
> erase are available and 4BAIT advertises them. We don't have to enter
> 4 byte address mode for read/program/erase.
btw. this is a pity. you are using the stateless 4b opcodes but
then you don't provide stateless opcodes for the read any register
op :/
> So, I think we need to differentiate between address length for
> read/program/erase and flash's default address mode.
Or we keep them in sync. E.g. switch to 4bytes mode if we are
using the 4 byte. Granted, that sounds like a hack :)
> Obviously we are using nor->addr_nbytes as address length for read/
> program/erase and should keep this usage.
Yes, I wasn't aware that we actually two different runtime
parameters:
- the read/program/erase address width, also used with the
4b opcodes
- internal mode 3b/4b. Up until now, this wasn't an issue
because either the mode was switched or the 4b opcodes
were used. So this was mutually exclusive. Now we have
flashes which uses 4b opcodes _and_ we need the state
of the internal mode.
I can't think of a good solution for now. Need to think
more about this, but I'm pretty busy at the moment.
What I think is clear is that we need two different modes
here in the spi_nor struct. nor->addr_nbytes for the
read/program/erase opcodes and nor->address_mode or similar
which tracks the SPI flash's internal address mode.
> For flash's default address mode, my preference is to use
> params->addr_nbytes, but I should rename it to something like
> params->def_addr_nbytes and rework spi_nor_set_addr_nbytes().
IMHO params should only be used to store the parsed (or
hardcoded) parameters.
-michael
> static int spi_nor_set_addr_nbytes(struct spi_nor *nor)
> {
> if (nor->flags & SNOR_F_HAS_4BAIT) {
> nor->addr_nbytes = 4;
> } else if (nor->params->def_addr_nbytes) {
> nor->addr_nbytes = nor->params->def_addr_nbytes;
>
>> At some point, the mode is switched and nor->addr_nbytes becomes
>> params->addr_nbytes. It seems in your case nor->addr_nbytes should
>> be 4 right from the beginning. Which also means nor->addr_nbytes
>> should be 3 for the other cases (and probably not 0).
>>
> With param->def_addr_nbytes, I think we can keep nor->addr_nbytes = 0
> during SFDP parse.
>
> Thanks,
> Takahiro
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
2022-05-23 7:49 ` Michael Walle
@ 2022-05-23 9:56 ` Takahiro Kuwano
2022-06-03 9:33 ` Takahiro Kuwano
2022-07-21 16:06 ` Tudor.Ambarus
1 sibling, 1 reply; 42+ messages in thread
From: Takahiro Kuwano @ 2022-05-23 9:56 UTC (permalink / raw)
To: Michael Walle
Cc: linux-mtd, tudor.ambarus, miquel.raynal, richard, vigneshr,
p.yadav, Bacem.Daassi, Takahiro Kuwano
On 5/23/2022 4:49 PM, Michael Walle wrote:
> Am 2022-05-14 05:51, schrieb Takahiro Kuwano:
>> On 5/13/2022 6:40 PM, Michael Walle wrote:
>>> [btw the subject still has the old name of the addr_width]
>>>
>> Yes, it must be fixed in next rev.
>>
>>> Am 2022-05-13 03:26, schrieb Takahiro Kuwano:
>>>> On 5/13/2022 7:14 AM, Michael Walle wrote:
>>>>> Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
>>>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>>>
>>>>>> In 4BAIT parse, keep nor->params->addr_width because it may be used as
>>>>>> current address mode in SMPT parse later on.
>>>>>
>>>>> Mh I'm not sure this is needed at all.
>>>>>
>>>>> SFDP spec says
>>>>> Variable address length (the current setting of the address
>>>>> length mode defines the address length)
>>>>>
>>>>> and
>>>>> When the length is defined as variable, the software or hardware
>>>>> controlling the memory is aware of the address length mode last
>>>>> set in the memory device and this same length of address.
>>>>>
>>>>> We don't set any address mode until all the SFDP parsing is
>>>>> over. Therefore we should always be in 3 byte mode, no?
>>>>>
>>>> Actually there are some devices that have variable address length but
>>>> 4 byte mode by default (I will work on those devices after this series
>>>> is settled). To support such case, I prefer to use params->addr_nbytes
>>>> as current address mode so that I can fix it in post_bfpt_fixup() hook.
>>>
>>> Are there public datasheets available? So these devices have a 3 byte
>> I will send datasheets to you in another email. At this point, only
>> summary datasheet is available in website.
>>
>>> and a 4 byte mode, but after reset, they are in the 4 byte mode? Looks
>> Yes.
>>
>>> like it should be fixed in a different way. I'm not sure the "current
>>> mode" handling is correct.
>>>
>> Yes, we may want to introduce a new flag like SPI_NOR_4BAM_DEFAULT and check
>> the flag in BFPT parse. Once I send another series, please review.
>>
>>> We need to differentiate between the mode the flash currently is using
>>> (nor->addr_nbytes) and the mode parsed by SFDP (params->addr_nbytes).
>>>
>> The flash's address mode affects the address length of Non-4B opcodes,
>> including read/write any register ops used in SMPT parse and Infineon
>> (spansion) specific hooks.
>>
>> The 4B opcodes always take address length of 4 regardless of flash's
>> address mode. In these Infineon chips, 4B opcodes for read/program/
>> erase are available and 4BAIT advertises them. We don't have to enter
>> 4 byte address mode for read/program/erase.
>
> btw. this is a pity. you are using the stateless 4b opcodes but
> then you don't provide stateless opcodes for the read any register
> op :/
>
>> So, I think we need to differentiate between address length for
>> read/program/erase and flash's default address mode.
>
> Or we keep them in sync. E.g. switch to 4bytes mode if we are
> using the 4 byte. Granted, that sounds like a hack :)
>
Yes, the earlier revision of the series switched to 4byte mode in fixup
hook, but we don't want to change device settings during SFDP parse.
>> Obviously we are using nor->addr_nbytes as address length for read/
>> program/erase and should keep this usage.
>
> Yes, I wasn't aware that we actually two different runtime
> parameters:
> - the read/program/erase address width, also used with the
> 4b opcodes
> - internal mode 3b/4b. Up until now, this wasn't an issue
> because either the mode was switched or the 4b opcodes
> were used. So this was mutually exclusive. Now we have
> flashes which uses 4b opcodes _and_ we need the state
> of the internal mode.
>
> I can't think of a good solution for now. Need to think
> more about this, but I'm pretty busy at the moment.
> What I think is clear is that we need two different modes
> here in the spi_nor struct. nor->addr_nbytes for the
> read/program/erase opcodes and nor->address_mode or similar
> which tracks the SPI flash's internal address mode.
>
So, until we come up with a good solution, can we directly use the address
length of 3 in SMPT parse and vendor specific hooks? By doing that, we can
remove this (6/8) and previous (5/8) patches (minimum change would be
better as a temporary solution).
>> For flash's default address mode, my preference is to use
>> params->addr_nbytes, but I should rename it to something like
>> params->def_addr_nbytes and rework spi_nor_set_addr_nbytes().
>
> IMHO params should only be used to store the parsed (or
> hardcoded) parameters.
>
OK, understood.
Thanks,
Takahiro
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
2022-05-23 9:56 ` Takahiro Kuwano
@ 2022-06-03 9:33 ` Takahiro Kuwano
0 siblings, 0 replies; 42+ messages in thread
From: Takahiro Kuwano @ 2022-06-03 9:33 UTC (permalink / raw)
To: Michael Walle, tudor.ambarus, p.yadav
Cc: linux-mtd, miquel.raynal, richard, vigneshr, Bacem.Daassi,
Takahiro Kuwano
Any thoughts?
On 5/23/2022 6:56 PM, Takahiro Kuwano wrote:
>
>
> On 5/23/2022 4:49 PM, Michael Walle wrote:
>> Am 2022-05-14 05:51, schrieb Takahiro Kuwano:
>>> On 5/13/2022 6:40 PM, Michael Walle wrote:
>>>> [btw the subject still has the old name of the addr_width]
>>>>
>>> Yes, it must be fixed in next rev.
>>>
>>>> Am 2022-05-13 03:26, schrieb Takahiro Kuwano:
>>>>> On 5/13/2022 7:14 AM, Michael Walle wrote:
>>>>>> Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
>>>>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>>>>
>>>>>>> In 4BAIT parse, keep nor->params->addr_width because it may be used as
>>>>>>> current address mode in SMPT parse later on.
>>>>>>
>>>>>> Mh I'm not sure this is needed at all.
>>>>>>
>>>>>> SFDP spec says
>>>>>> Variable address length (the current setting of the address
>>>>>> length mode defines the address length)
>>>>>>
>>>>>> and
>>>>>> When the length is defined as variable, the software or hardware
>>>>>> controlling the memory is aware of the address length mode last
>>>>>> set in the memory device and this same length of address.
>>>>>>
>>>>>> We don't set any address mode until all the SFDP parsing is
>>>>>> over. Therefore we should always be in 3 byte mode, no?
>>>>>>
>>>>> Actually there are some devices that have variable address length but
>>>>> 4 byte mode by default (I will work on those devices after this series
>>>>> is settled). To support such case, I prefer to use params->addr_nbytes
>>>>> as current address mode so that I can fix it in post_bfpt_fixup() hook.
>>>>
>>>> Are there public datasheets available? So these devices have a 3 byte
>>> I will send datasheets to you in another email. At this point, only
>>> summary datasheet is available in website.
>>>
>>>> and a 4 byte mode, but after reset, they are in the 4 byte mode? Looks
>>> Yes.
>>>
>>>> like it should be fixed in a different way. I'm not sure the "current
>>>> mode" handling is correct.
>>>>
>>> Yes, we may want to introduce a new flag like SPI_NOR_4BAM_DEFAULT and check
>>> the flag in BFPT parse. Once I send another series, please review.
>>>
>>>> We need to differentiate between the mode the flash currently is using
>>>> (nor->addr_nbytes) and the mode parsed by SFDP (params->addr_nbytes).
>>>>
>>> The flash's address mode affects the address length of Non-4B opcodes,
>>> including read/write any register ops used in SMPT parse and Infineon
>>> (spansion) specific hooks.
>>>
>>> The 4B opcodes always take address length of 4 regardless of flash's
>>> address mode. In these Infineon chips, 4B opcodes for read/program/
>>> erase are available and 4BAIT advertises them. We don't have to enter
>>> 4 byte address mode for read/program/erase.
>>
>> btw. this is a pity. you are using the stateless 4b opcodes but
>> then you don't provide stateless opcodes for the read any register
>> op :/
>>
>>> So, I think we need to differentiate between address length for
>>> read/program/erase and flash's default address mode.
>>
>> Or we keep them in sync. E.g. switch to 4bytes mode if we are
>> using the 4 byte. Granted, that sounds like a hack :)
>>
> Yes, the earlier revision of the series switched to 4byte mode in fixup
> hook, but we don't want to change device settings during SFDP parse.
>
>>> Obviously we are using nor->addr_nbytes as address length for read/
>>> program/erase and should keep this usage.
>>
>> Yes, I wasn't aware that we actually two different runtime
>> parameters:
>> - the read/program/erase address width, also used with the
>> 4b opcodes
>> - internal mode 3b/4b. Up until now, this wasn't an issue
>> because either the mode was switched or the 4b opcodes
>> were used. So this was mutually exclusive. Now we have
>> flashes which uses 4b opcodes _and_ we need the state
>> of the internal mode.
>>
>> I can't think of a good solution for now. Need to think
>> more about this, but I'm pretty busy at the moment.
>> What I think is clear is that we need two different modes
>> here in the spi_nor struct. nor->addr_nbytes for the
>> read/program/erase opcodes and nor->address_mode or similar
>> which tracks the SPI flash's internal address mode.
>>
> So, until we come up with a good solution, can we directly use the address
> length of 3 in SMPT parse and vendor specific hooks? By doing that, we can
> remove this (6/8) and previous (5/8) patches (minimum change would be
> better as a temporary solution).
>
>>> For flash's default address mode, my preference is to use
>>> params->addr_nbytes, but I should rename it to something like
>>> params->def_addr_nbytes and rework spi_nor_set_addr_nbytes().
>>
>> IMHO params should only be used to store the parsed (or
>> hardcoded) parameters.
>>
> OK, understood.
>
> Thanks,
> Takahiro
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
2022-05-23 7:49 ` Michael Walle
2022-05-23 9:56 ` Takahiro Kuwano
@ 2022-07-21 16:06 ` Tudor.Ambarus
2022-07-22 4:00 ` Takahiro Kuwano
1 sibling, 1 reply; 42+ messages in thread
From: Tudor.Ambarus @ 2022-07-21 16:06 UTC (permalink / raw)
To: michael, tkuw584924
Cc: linux-mtd, miquel.raynal, richard, vigneshr, p.yadav,
Bacem.Daassi, Takahiro.Kuwano
On 5/23/22 10:49, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Am 2022-05-14 05:51, schrieb Takahiro Kuwano:
>> On 5/13/2022 6:40 PM, Michael Walle wrote:
>>> [btw the subject still has the old name of the addr_width]
>>>
>> Yes, it must be fixed in next rev.
>>
>>> Am 2022-05-13 03:26, schrieb Takahiro Kuwano:
>>>> On 5/13/2022 7:14 AM, Michael Walle wrote:
>>>>> Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
>>>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>>>
>>>>>> In 4BAIT parse, keep nor->params->addr_width because it may be used
>>>>>> as
>>>>>> current address mode in SMPT parse later on.
>>>>>
>>>>> Mh I'm not sure this is needed at all.
>>>>>
>>>>> SFDP spec says
>>>>> Variable address length (the current setting of the address
>>>>> length mode defines the address length)
>>>>>
>>>>> and
>>>>> When the length is defined as variable, the software or hardware
>>>>> controlling the memory is aware of the address length mode last
>>>>> set in the memory device and this same length of address.
>>>>>
>>>>> We don't set any address mode until all the SFDP parsing is
>>>>> over. Therefore we should always be in 3 byte mode, no?
>>>>>
>>>> Actually there are some devices that have variable address length but
>>>> 4 byte mode by default (I will work on those devices after this
>>>> series
>>>> is settled). To support such case, I prefer to use
>>>> params->addr_nbytes
>>>> as current address mode so that I can fix it in post_bfpt_fixup()
>>>> hook.
>>>
>>> Are there public datasheets available? So these devices have a 3 byte
>> I will send datasheets to you in another email. At this point, only
>> summary datasheet is available in website.
>>
>>> and a 4 byte mode, but after reset, they are in the 4 byte mode? Looks
>> Yes.
>>
>>> like it should be fixed in a different way. I'm not sure the "current
>>> mode" handling is correct.
>>>
>> Yes, we may want to introduce a new flag like SPI_NOR_4BAM_DEFAULT and
>> check
>> the flag in BFPT parse. Once I send another series, please review.
>>
>>> We need to differentiate between the mode the flash currently is using
>>> (nor->addr_nbytes) and the mode parsed by SFDP (params->addr_nbytes).
>>>
>> The flash's address mode affects the address length of Non-4B opcodes,
>> including read/write any register ops used in SMPT parse and Infineon
>> (spansion) specific hooks.
>>
>> The 4B opcodes always take address length of 4 regardless of flash's
>> address mode. In these Infineon chips, 4B opcodes for read/program/
>> erase are available and 4BAIT advertises them. We don't have to enter
>> 4 byte address mode for read/program/erase.
>
> btw. this is a pity. you are using the stateless 4b opcodes but
> then you don't provide stateless opcodes for the read any register
> op :/
>
>> So, I think we need to differentiate between address length for
>> read/program/erase and flash's default address mode.
>
> Or we keep them in sync. E.g. switch to 4bytes mode if we are
> using the 4 byte. Granted, that sounds like a hack :)
>
>> Obviously we are using nor->addr_nbytes as address length for read/
>> program/erase and should keep this usage.
>
> Yes, I wasn't aware that we actually two different runtime
> parameters:
> - the read/program/erase address width, also used with the
> 4b opcodes
> - internal mode 3b/4b. Up until now, this wasn't an issue
> because either the mode was switched or the 4b opcodes
> were used. So this was mutually exclusive. Now we have
> flashes which uses 4b opcodes _and_ we need the state
> of the internal mode.
>
> I can't think of a good solution for now. Need to think
> more about this, but I'm pretty busy at the moment.
> What I think is clear is that we need two different modes
> here in the spi_nor struct. nor->addr_nbytes for the
> read/program/erase opcodes and nor->address_mode or similar
> which tracks the SPI flash's internal address mode.
Hi, Takahiro,
Can we determine the flash's internal address mode by querying
the flash at run-time? Is this possible on Semper flashes?
To extend the idea, when the flash supports BFPT and defines
BFPT_DWORD1_ADDRESS_BYTES_3_ONLY or BFPT_DWORD1_ADDRESS_BYTES_4_ONLY
the things are clear, addr_nbytes is fixed.
If the flash does not define BFPT or uses
BFPT_DWORD1_ADDRESS_BYTES_3_OR_4, then we could determine the addr
mode by querying the flash.
>
>> For flash's default address mode, my preference is to use
>> params->addr_nbytes, but I should rename it to something like
>> params->def_addr_nbytes and rework spi_nor_set_addr_nbytes().
>
> IMHO params should only be used to store the parsed (or
> hardcoded) parameters.
yep, I share Michael's opinion.
Cheers,
ta
>
> -michael
>
>> static int spi_nor_set_addr_nbytes(struct spi_nor *nor)
>> {
>> if (nor->flags & SNOR_F_HAS_4BAIT) {
>> nor->addr_nbytes = 4;
>> } else if (nor->params->def_addr_nbytes) {
>> nor->addr_nbytes = nor->params->def_addr_nbytes;
>>
>>> At some point, the mode is switched and nor->addr_nbytes becomes
>>> params->addr_nbytes. It seems in your case nor->addr_nbytes should
>>> be 4 right from the beginning. Which also means nor->addr_nbytes
>>> should be 3 for the other cases (and probably not 0).
>>>
>> With param->def_addr_nbytes, I think we can keep nor->addr_nbytes = 0
>> during SFDP parse.
>>
>> Thanks,
>> Takahiro
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
2022-07-21 16:06 ` Tudor.Ambarus
@ 2022-07-22 4:00 ` Takahiro Kuwano
2022-07-22 4:20 ` Tudor.Ambarus
0 siblings, 1 reply; 42+ messages in thread
From: Takahiro Kuwano @ 2022-07-22 4:00 UTC (permalink / raw)
To: Tudor.Ambarus, michael
Cc: linux-mtd, miquel.raynal, richard, vigneshr, p.yadav,
Bacem.Daassi, Takahiro.Kuwano
On 7/22/2022 1:06 AM, Tudor.Ambarus@microchip.com wrote:
> On 5/23/22 10:49, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> Am 2022-05-14 05:51, schrieb Takahiro Kuwano:
>>> On 5/13/2022 6:40 PM, Michael Walle wrote:
>>>> [btw the subject still has the old name of the addr_width]
>>>>
>>> Yes, it must be fixed in next rev.
>>>
>>>> Am 2022-05-13 03:26, schrieb Takahiro Kuwano:
>>>>> On 5/13/2022 7:14 AM, Michael Walle wrote:
>>>>>> Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
>>>>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>>>>
>>>>>>> In 4BAIT parse, keep nor->params->addr_width because it may be used
>>>>>>> as
>>>>>>> current address mode in SMPT parse later on.
>>>>>>
>>>>>> Mh I'm not sure this is needed at all.
>>>>>>
>>>>>> SFDP spec says
>>>>>> Variable address length (the current setting of the address
>>>>>> length mode defines the address length)
>>>>>>
>>>>>> and
>>>>>> When the length is defined as variable, the software or hardware
>>>>>> controlling the memory is aware of the address length mode last
>>>>>> set in the memory device and this same length of address.
>>>>>>
>>>>>> We don't set any address mode until all the SFDP parsing is
>>>>>> over. Therefore we should always be in 3 byte mode, no?
>>>>>>
>>>>> Actually there are some devices that have variable address length but
>>>>> 4 byte mode by default (I will work on those devices after this
>>>>> series
>>>>> is settled). To support such case, I prefer to use
>>>>> params->addr_nbytes
>>>>> as current address mode so that I can fix it in post_bfpt_fixup()
>>>>> hook.
>>>>
>>>> Are there public datasheets available? So these devices have a 3 byte
>>> I will send datasheets to you in another email. At this point, only
>>> summary datasheet is available in website.
>>>
>>>> and a 4 byte mode, but after reset, they are in the 4 byte mode? Looks
>>> Yes.
>>>
>>>> like it should be fixed in a different way. I'm not sure the "current
>>>> mode" handling is correct.
>>>>
>>> Yes, we may want to introduce a new flag like SPI_NOR_4BAM_DEFAULT and
>>> check
>>> the flag in BFPT parse. Once I send another series, please review.
>>>
>>>> We need to differentiate between the mode the flash currently is using
>>>> (nor->addr_nbytes) and the mode parsed by SFDP (params->addr_nbytes).
>>>>
>>> The flash's address mode affects the address length of Non-4B opcodes,
>>> including read/write any register ops used in SMPT parse and Infineon
>>> (spansion) specific hooks.
>>>
>>> The 4B opcodes always take address length of 4 regardless of flash's
>>> address mode. In these Infineon chips, 4B opcodes for read/program/
>>> erase are available and 4BAIT advertises them. We don't have to enter
>>> 4 byte address mode for read/program/erase.
>>
>> btw. this is a pity. you are using the stateless 4b opcodes but
>> then you don't provide stateless opcodes for the read any register
>> op :/
>>
>>> So, I think we need to differentiate between address length for
>>> read/program/erase and flash's default address mode.
>>
>> Or we keep them in sync. E.g. switch to 4bytes mode if we are
>> using the 4 byte. Granted, that sounds like a hack :)
>>
>>> Obviously we are using nor->addr_nbytes as address length for read/
>>> program/erase and should keep this usage.
>>
>> Yes, I wasn't aware that we actually two different runtime
>> parameters:
>> - the read/program/erase address width, also used with the
>> 4b opcodes
>> - internal mode 3b/4b. Up until now, this wasn't an issue
>> because either the mode was switched or the 4b opcodes
>> were used. So this was mutually exclusive. Now we have
>> flashes which uses 4b opcodes _and_ we need the state
>> of the internal mode.
>>
>> I can't think of a good solution for now. Need to think
>> more about this, but I'm pretty busy at the moment.
>> What I think is clear is that we need two different modes
>> here in the spi_nor struct. nor->addr_nbytes for the
>> read/program/erase opcodes and nor->address_mode or similar
>> which tracks the SPI flash's internal address mode.
>
> Hi, Takahiro,
>
> Can we determine the flash's internal address mode by querying
> the flash at run-time? Is this possible on Semper flashes?
>
CFR2V[7] has current address mode, but to read that, we need to
issue Read Any Register which address length relies on current
address mode. Chicken-and-egg...
> To extend the idea, when the flash supports BFPT and defines
> BFPT_DWORD1_ADDRESS_BYTES_3_ONLY or BFPT_DWORD1_ADDRESS_BYTES_4_ONLY
> the things are clear, addr_nbytes is fixed.
> If the flash does not define BFPT or uses
> BFPT_DWORD1_ADDRESS_BYTES_3_OR_4, then we could determine the addr
> mode by querying the flash.
>
>>
>>> For flash's default address mode, my preference is to use
>>> params->addr_nbytes, but I should rename it to something like
>>> params->def_addr_nbytes and rework spi_nor_set_addr_nbytes().
>>
>> IMHO params should only be used to store the parsed (or
>> hardcoded) parameters.
>
> yep, I share Michael's opinion.
>
> Cheers,
> ta
>
>>
>> -michael
>>
>>> static int spi_nor_set_addr_nbytes(struct spi_nor *nor)
>>> {
>>> if (nor->flags & SNOR_F_HAS_4BAIT) {
>>> nor->addr_nbytes = 4;
>>> } else if (nor->params->def_addr_nbytes) {
>>> nor->addr_nbytes = nor->params->def_addr_nbytes;
>>>
>>>> At some point, the mode is switched and nor->addr_nbytes becomes
>>>> params->addr_nbytes. It seems in your case nor->addr_nbytes should
>>>> be 4 right from the beginning. Which also means nor->addr_nbytes
>>>> should be 3 for the other cases (and probably not 0).
>>>>
>>> With param->def_addr_nbytes, I think we can keep nor->addr_nbytes = 0
>>> during SFDP parse.
>>>
>>> Thanks,
>>> Takahiro
>
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
2022-07-22 4:00 ` Takahiro Kuwano
@ 2022-07-22 4:20 ` Tudor.Ambarus
2022-07-22 4:31 ` Vanessa Page
2022-07-22 4:46 ` Takahiro Kuwano
0 siblings, 2 replies; 42+ messages in thread
From: Tudor.Ambarus @ 2022-07-22 4:20 UTC (permalink / raw)
To: tkuw584924, michael
Cc: linux-mtd, miquel.raynal, richard, vigneshr, p.yadav,
Bacem.Daassi, Takahiro.Kuwano
On 7/22/22 07:00, Takahiro Kuwano wrote:
Good morning, Takahiro!
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 7/22/2022 1:06 AM, Tudor.Ambarus@microchip.com wrote:
>> On 5/23/22 10:49, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> Am 2022-05-14 05:51, schrieb Takahiro Kuwano:
>>>> On 5/13/2022 6:40 PM, Michael Walle wrote:
>>>>> [btw the subject still has the old name of the addr_width]
>>>>>
>>>> Yes, it must be fixed in next rev.
>>>>
>>>>> Am 2022-05-13 03:26, schrieb Takahiro Kuwano:
>>>>>> On 5/13/2022 7:14 AM, Michael Walle wrote:
>>>>>>> Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
>>>>>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>>>>>
>>>>>>>> In 4BAIT parse, keep nor->params->addr_width because it may be used
>>>>>>>> as
>>>>>>>> current address mode in SMPT parse later on.
>>>>>>>
>>>>>>> Mh I'm not sure this is needed at all.
>>>>>>>
>>>>>>> SFDP spec says
>>>>>>> Variable address length (the current setting of the address
>>>>>>> length mode defines the address length)
>>>>>>>
>>>>>>> and
>>>>>>> When the length is defined as variable, the software or hardware
>>>>>>> controlling the memory is aware of the address length mode last
>>>>>>> set in the memory device and this same length of address.
>>>>>>>
>>>>>>> We don't set any address mode until all the SFDP parsing is
>>>>>>> over. Therefore we should always be in 3 byte mode, no?
>>>>>>>
>>>>>> Actually there are some devices that have variable address length but
>>>>>> 4 byte mode by default (I will work on those devices after this
>>>>>> series
>>>>>> is settled). To support such case, I prefer to use
>>>>>> params->addr_nbytes
>>>>>> as current address mode so that I can fix it in post_bfpt_fixup()
>>>>>> hook.
>>>>>
>>>>> Are there public datasheets available? So these devices have a 3 byte
>>>> I will send datasheets to you in another email. At this point, only
>>>> summary datasheet is available in website.
>>>>
>>>>> and a 4 byte mode, but after reset, they are in the 4 byte mode? Looks
>>>> Yes.
>>>>
>>>>> like it should be fixed in a different way. I'm not sure the "current
>>>>> mode" handling is correct.
>>>>>
>>>> Yes, we may want to introduce a new flag like SPI_NOR_4BAM_DEFAULT and
>>>> check
>>>> the flag in BFPT parse. Once I send another series, please review.
>>>>
>>>>> We need to differentiate between the mode the flash currently is using
>>>>> (nor->addr_nbytes) and the mode parsed by SFDP (params->addr_nbytes).
>>>>>
>>>> The flash's address mode affects the address length of Non-4B opcodes,
>>>> including read/write any register ops used in SMPT parse and Infineon
>>>> (spansion) specific hooks.
>>>>
>>>> The 4B opcodes always take address length of 4 regardless of flash's
>>>> address mode. In these Infineon chips, 4B opcodes for read/program/
>>>> erase are available and 4BAIT advertises them. We don't have to enter
>>>> 4 byte address mode for read/program/erase.
>>>
>>> btw. this is a pity. you are using the stateless 4b opcodes but
>>> then you don't provide stateless opcodes for the read any register
>>> op :/
>>>
>>>> So, I think we need to differentiate between address length for
>>>> read/program/erase and flash's default address mode.
>>>
>>> Or we keep them in sync. E.g. switch to 4bytes mode if we are
>>> using the 4 byte. Granted, that sounds like a hack :)
>>>
>>>> Obviously we are using nor->addr_nbytes as address length for read/
>>>> program/erase and should keep this usage.
>>>
>>> Yes, I wasn't aware that we actually two different runtime
>>> parameters:
>>> - the read/program/erase address width, also used with the
>>> 4b opcodes
>>> - internal mode 3b/4b. Up until now, this wasn't an issue
>>> because either the mode was switched or the 4b opcodes
>>> were used. So this was mutually exclusive. Now we have
>>> flashes which uses 4b opcodes _and_ we need the state
>>> of the internal mode.
>>>
>>> I can't think of a good solution for now. Need to think
>>> more about this, but I'm pretty busy at the moment.
>>> What I think is clear is that we need two different modes
>>> here in the spi_nor struct. nor->addr_nbytes for the
>>> read/program/erase opcodes and nor->address_mode or similar
>>> which tracks the SPI flash's internal address mode.
>>
>> Hi, Takahiro,
>>
>> Can we determine the flash's internal address mode by querying
>> the flash at run-time? Is this possible on Semper flashes?
>>
> CFR2V[7] has current address mode, but to read that, we need to
> issue Read Any Register which address length relies on current
> address mode. Chicken-and-egg...
>
I see. What happens if we issue the Read Any Register command with
the wrong address internal mode? Will I read just 0xff?
For example try reading CFR2V[7] using addr_nbytes of value 3, and
if it fails, to read it again but this time using addr_nbytes of
value 4.
Cheers,
ta
>> To extend the idea, when the flash supports BFPT and defines
>> BFPT_DWORD1_ADDRESS_BYTES_3_ONLY or BFPT_DWORD1_ADDRESS_BYTES_4_ONLY
>> the things are clear, addr_nbytes is fixed.
>> If the flash does not define BFPT or uses
>> BFPT_DWORD1_ADDRESS_BYTES_3_OR_4, then we could determine the addr
>> mode by querying the flash.
>>
>>>
>>>> For flash's default address mode, my preference is to use
>>>> params->addr_nbytes, but I should rename it to something like
>>>> params->def_addr_nbytes and rework spi_nor_set_addr_nbytes().
>>>
>>> IMHO params should only be used to store the parsed (or
>>> hardcoded) parameters.
>>
>> yep, I share Michael's opinion.
>>
>> Cheers,
>> ta
>>
>>>
>>> -michael
>>>
>>>> static int spi_nor_set_addr_nbytes(struct spi_nor *nor)
>>>> {
>>>> if (nor->flags & SNOR_F_HAS_4BAIT) {
>>>> nor->addr_nbytes = 4;
>>>> } else if (nor->params->def_addr_nbytes) {
>>>> nor->addr_nbytes = nor->params->def_addr_nbytes;
>>>>
>>>>> At some point, the mode is switched and nor->addr_nbytes becomes
>>>>> params->addr_nbytes. It seems in your case nor->addr_nbytes should
>>>>> be 4 right from the beginning. Which also means nor->addr_nbytes
>>>>> should be 3 for the other cases (and probably not 0).
>>>>>
>>>> With param->def_addr_nbytes, I think we can keep nor->addr_nbytes = 0
>>>> during SFDP parse.
>>>>
>>>> Thanks,
>>>> Takahiro
>>
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
2022-07-22 4:20 ` Tudor.Ambarus
@ 2022-07-22 4:31 ` Vanessa Page
2022-07-22 4:46 ` Takahiro Kuwano
1 sibling, 0 replies; 42+ messages in thread
From: Vanessa Page @ 2022-07-22 4:31 UTC (permalink / raw)
To: Tudor.Ambarus
Cc: tkuw584924, michael, linux-mtd, miquel.raynal, richard, vigneshr,
p.yadav, Bacem.Daassi, Takahiro.Kuwano
I have repeatedly told you to stop contacting this email.
> On Jul 22, 2022, at 12:23 AM, Tudor.Ambarus@microchip.com wrote:
>
> On 7/22/22 07:00, Takahiro Kuwano wrote:
>
> Good morning, Takahiro!
>
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>>> On 7/22/2022 1:06 AM, Tudor.Ambarus@microchip.com wrote:
>>> On 5/23/22 10:49, Michael Walle wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> Am 2022-05-14 05:51, schrieb Takahiro Kuwano:
>>>>> On 5/13/2022 6:40 PM, Michael Walle wrote:
>>>>>> [btw the subject still has the old name of the addr_width]
>>>>>>
>>>>> Yes, it must be fixed in next rev.
>>>>>
>>>>>> Am 2022-05-13 03:26, schrieb Takahiro Kuwano:
>>>>>>> On 5/13/2022 7:14 AM, Michael Walle wrote:
>>>>>>>> Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
>>>>>>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>>>>>>
>>>>>>>>> In 4BAIT parse, keep nor->params->addr_width because it may be used
>>>>>>>>> as
>>>>>>>>> current address mode in SMPT parse later on.
>>>>>>>>
>>>>>>>> Mh I'm not sure this is needed at all.
>>>>>>>>
>>>>>>>> SFDP spec says
>>>>>>>> Variable address length (the current setting of the address
>>>>>>>> length mode defines the address length)
>>>>>>>>
>>>>>>>> and
>>>>>>>> When the length is defined as variable, the software or hardware
>>>>>>>> controlling the memory is aware of the address length mode last
>>>>>>>> set in the memory device and this same length of address.
>>>>>>>>
>>>>>>>> We don't set any address mode until all the SFDP parsing is
>>>>>>>> over. Therefore we should always be in 3 byte mode, no?
>>>>>>>>
>>>>>>> Actually there are some devices that have variable address length but
>>>>>>> 4 byte mode by default (I will work on those devices after this
>>>>>>> series
>>>>>>> is settled). To support such case, I prefer to use
>>>>>>> params->addr_nbytes
>>>>>>> as current address mode so that I can fix it in post_bfpt_fixup()
>>>>>>> hook.
>>>>>>
>>>>>> Are there public datasheets available? So these devices have a 3 byte
>>>>> I will send datasheets to you in another email. At this point, only
>>>>> summary datasheet is available in website.
>>>>>
>>>>>> and a 4 byte mode, but after reset, they are in the 4 byte mode? Looks
>>>>> Yes.
>>>>>
>>>>>> like it should be fixed in a different way. I'm not sure the "current
>>>>>> mode" handling is correct.
>>>>>>
>>>>> Yes, we may want to introduce a new flag like SPI_NOR_4BAM_DEFAULT and
>>>>> check
>>>>> the flag in BFPT parse. Once I send another series, please review.
>>>>>
>>>>>> We need to differentiate between the mode the flash currently is using
>>>>>> (nor->addr_nbytes) and the mode parsed by SFDP (params->addr_nbytes).
>>>>>>
>>>>> The flash's address mode affects the address length of Non-4B opcodes,
>>>>> including read/write any register ops used in SMPT parse and Infineon
>>>>> (spansion) specific hooks.
>>>>>
>>>>> The 4B opcodes always take address length of 4 regardless of flash's
>>>>> address mode. In these Infineon chips, 4B opcodes for read/program/
>>>>> erase are available and 4BAIT advertises them. We don't have to enter
>>>>> 4 byte address mode for read/program/erase.
>>>>
>>>> btw. this is a pity. you are using the stateless 4b opcodes but
>>>> then you don't provide stateless opcodes for the read any register
>>>> op :/
>>>>
>>>>> So, I think we need to differentiate between address length for
>>>>> read/program/erase and flash's default address mode.
>>>>
>>>> Or we keep them in sync. E.g. switch to 4bytes mode if we are
>>>> using the 4 byte. Granted, that sounds like a hack :)
>>>>
>>>>> Obviously we are using nor->addr_nbytes as address length for read/
>>>>> program/erase and should keep this usage.
>>>>
>>>> Yes, I wasn't aware that we actually two different runtime
>>>> parameters:
>>>> - the read/program/erase address width, also used with the
>>>> 4b opcodes
>>>> - internal mode 3b/4b. Up until now, this wasn't an issue
>>>> because either the mode was switched or the 4b opcodes
>>>> were used. So this was mutually exclusive. Now we have
>>>> flashes which uses 4b opcodes _and_ we need the state
>>>> of the internal mode.
>>>>
>>>> I can't think of a good solution for now. Need to think
>>>> more about this, but I'm pretty busy at the moment.
>>>> What I think is clear is that we need two different modes
>>>> here in the spi_nor struct. nor->addr_nbytes for the
>>>> read/program/erase opcodes and nor->address_mode or similar
>>>> which tracks the SPI flash's internal address mode.
>>>
>>> Hi, Takahiro,
>>>
>>> Can we determine the flash's internal address mode by querying
>>> the flash at run-time? Is this possible on Semper flashes?
>>>
>> CFR2V[7] has current address mode, but to read that, we need to
>> issue Read Any Register which address length relies on current
>> address mode. Chicken-and-egg...
>>
>
> I see. What happens if we issue the Read Any Register command with
> the wrong address internal mode? Will I read just 0xff?
> For example try reading CFR2V[7] using addr_nbytes of value 3, and
> if it fails, to read it again but this time using addr_nbytes of
> value 4.
>
> Cheers,
> ta
>
>
>>> To extend the idea, when the flash supports BFPT and defines
>>> BFPT_DWORD1_ADDRESS_BYTES_3_ONLY or BFPT_DWORD1_ADDRESS_BYTES_4_ONLY
>>> the things are clear, addr_nbytes is fixed.
>>> If the flash does not define BFPT or uses
>>> BFPT_DWORD1_ADDRESS_BYTES_3_OR_4, then we could determine the addr
>>> mode by querying the flash.
>>>
>>>>
>>>>> For flash's default address mode, my preference is to use
>>>>> params->addr_nbytes, but I should rename it to something like
>>>>> params->def_addr_nbytes and rework spi_nor_set_addr_nbytes().
>>>>
>>>> IMHO params should only be used to store the parsed (or
>>>> hardcoded) parameters.
>>>
>>> yep, I share Michael's opinion.
>>>
>>> Cheers,
>>> ta
>>>
>>>>
>>>> -michael
>>>>
>>>>> static int spi_nor_set_addr_nbytes(struct spi_nor *nor)
>>>>> {
>>>>> if (nor->flags & SNOR_F_HAS_4BAIT) {
>>>>> nor->addr_nbytes = 4;
>>>>> } else if (nor->params->def_addr_nbytes) {
>>>>> nor->addr_nbytes = nor->params->def_addr_nbytes;
>>>>>
>>>>>> At some point, the mode is switched and nor->addr_nbytes becomes
>>>>>> params->addr_nbytes. It seems in your case nor->addr_nbytes should
>>>>>> be 4 right from the beginning. Which also means nor->addr_nbytes
>>>>>> should be 3 for the other cases (and probably not 0).
>>>>>>
>>>>> With param->def_addr_nbytes, I think we can keep nor->addr_nbytes = 0
>>>>> during SFDP parse.
>>>>>
>>>>> Thanks,
>>>>> Takahiro
>>>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
2022-07-22 4:20 ` Tudor.Ambarus
2022-07-22 4:31 ` Vanessa Page
@ 2022-07-22 4:46 ` Takahiro Kuwano
2022-07-22 5:06 ` Tudor.Ambarus
1 sibling, 1 reply; 42+ messages in thread
From: Takahiro Kuwano @ 2022-07-22 4:46 UTC (permalink / raw)
To: Tudor.Ambarus, michael
Cc: linux-mtd, miquel.raynal, richard, vigneshr, p.yadav,
Bacem.Daassi, Takahiro.Kuwano
On 7/22/2022 1:20 PM, Tudor.Ambarus@microchip.com wrote:
> On 7/22/22 07:00, Takahiro Kuwano wrote:
>
> Good morning, Takahiro!
>
Good morning, Tudor!
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 7/22/2022 1:06 AM, Tudor.Ambarus@microchip.com wrote:
>>> On 5/23/22 10:49, Michael Walle wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> Am 2022-05-14 05:51, schrieb Takahiro Kuwano:
>>>>> On 5/13/2022 6:40 PM, Michael Walle wrote:
>>>>>> [btw the subject still has the old name of the addr_width]
>>>>>>
>>>>> Yes, it must be fixed in next rev.
>>>>>
>>>>>> Am 2022-05-13 03:26, schrieb Takahiro Kuwano:
>>>>>>> On 5/13/2022 7:14 AM, Michael Walle wrote:
>>>>>>>> Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
>>>>>>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>>>>>>
>>>>>>>>> In 4BAIT parse, keep nor->params->addr_width because it may be used
>>>>>>>>> as
>>>>>>>>> current address mode in SMPT parse later on.
>>>>>>>>
>>>>>>>> Mh I'm not sure this is needed at all.
>>>>>>>>
>>>>>>>> SFDP spec says
>>>>>>>> Variable address length (the current setting of the address
>>>>>>>> length mode defines the address length)
>>>>>>>>
>>>>>>>> and
>>>>>>>> When the length is defined as variable, the software or hardware
>>>>>>>> controlling the memory is aware of the address length mode last
>>>>>>>> set in the memory device and this same length of address.
>>>>>>>>
>>>>>>>> We don't set any address mode until all the SFDP parsing is
>>>>>>>> over. Therefore we should always be in 3 byte mode, no?
>>>>>>>>
>>>>>>> Actually there are some devices that have variable address length but
>>>>>>> 4 byte mode by default (I will work on those devices after this
>>>>>>> series
>>>>>>> is settled). To support such case, I prefer to use
>>>>>>> params->addr_nbytes
>>>>>>> as current address mode so that I can fix it in post_bfpt_fixup()
>>>>>>> hook.
>>>>>>
>>>>>> Are there public datasheets available? So these devices have a 3 byte
>>>>> I will send datasheets to you in another email. At this point, only
>>>>> summary datasheet is available in website.
>>>>>
>>>>>> and a 4 byte mode, but after reset, they are in the 4 byte mode? Looks
>>>>> Yes.
>>>>>
>>>>>> like it should be fixed in a different way. I'm not sure the "current
>>>>>> mode" handling is correct.
>>>>>>
>>>>> Yes, we may want to introduce a new flag like SPI_NOR_4BAM_DEFAULT and
>>>>> check
>>>>> the flag in BFPT parse. Once I send another series, please review.
>>>>>
>>>>>> We need to differentiate between the mode the flash currently is using
>>>>>> (nor->addr_nbytes) and the mode parsed by SFDP (params->addr_nbytes).
>>>>>>
>>>>> The flash's address mode affects the address length of Non-4B opcodes,
>>>>> including read/write any register ops used in SMPT parse and Infineon
>>>>> (spansion) specific hooks.
>>>>>
>>>>> The 4B opcodes always take address length of 4 regardless of flash's
>>>>> address mode. In these Infineon chips, 4B opcodes for read/program/
>>>>> erase are available and 4BAIT advertises them. We don't have to enter
>>>>> 4 byte address mode for read/program/erase.
>>>>
>>>> btw. this is a pity. you are using the stateless 4b opcodes but
>>>> then you don't provide stateless opcodes for the read any register
>>>> op :/
>>>>
>>>>> So, I think we need to differentiate between address length for
>>>>> read/program/erase and flash's default address mode.
>>>>
>>>> Or we keep them in sync. E.g. switch to 4bytes mode if we are
>>>> using the 4 byte. Granted, that sounds like a hack :)
>>>>
>>>>> Obviously we are using nor->addr_nbytes as address length for read/
>>>>> program/erase and should keep this usage.
>>>>
>>>> Yes, I wasn't aware that we actually two different runtime
>>>> parameters:
>>>> - the read/program/erase address width, also used with the
>>>> 4b opcodes
>>>> - internal mode 3b/4b. Up until now, this wasn't an issue
>>>> because either the mode was switched or the 4b opcodes
>>>> were used. So this was mutually exclusive. Now we have
>>>> flashes which uses 4b opcodes _and_ we need the state
>>>> of the internal mode.
>>>>
>>>> I can't think of a good solution for now. Need to think
>>>> more about this, but I'm pretty busy at the moment.
>>>> What I think is clear is that we need two different modes
>>>> here in the spi_nor struct. nor->addr_nbytes for the
>>>> read/program/erase opcodes and nor->address_mode or similar
>>>> which tracks the SPI flash's internal address mode.
>>>
>>> Hi, Takahiro,
>>>
>>> Can we determine the flash's internal address mode by querying
>>> the flash at run-time? Is this possible on Semper flashes?
>>>
>> CFR2V[7] has current address mode, but to read that, we need to
>> issue Read Any Register which address length relies on current
>> address mode. Chicken-and-egg...
>>
>
> I see. What happens if we issue the Read Any Register command with
> the wrong address internal mode? Will I read just 0xff?
> For example try reading CFR2V[7] using addr_nbytes of value 3, and
> if it fails, to read it again but this time using addr_nbytes of
> value 4.
>
It's undetermined. In case we issue Read Any Register with 3B address,
the host controller outputs 4 bytes (opcode + address) then inputs
1 byte. If the device is in 4B address mode at this time, the host
controller inputs the 1 byte while device is still waits for LSB of
address so IO is not driven by device.
Thanks,
Takahiro
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
2022-07-22 4:46 ` Takahiro Kuwano
@ 2022-07-22 5:06 ` Tudor.Ambarus
2022-07-22 5:11 ` Takahiro Kuwano
0 siblings, 1 reply; 42+ messages in thread
From: Tudor.Ambarus @ 2022-07-22 5:06 UTC (permalink / raw)
To: tkuw584924, michael
Cc: linux-mtd, miquel.raynal, richard, vigneshr, p.yadav,
Bacem.Daassi, Takahiro.Kuwano
On 7/22/22 07:46, Takahiro Kuwano wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 7/22/2022 1:20 PM, Tudor.Ambarus@microchip.com wrote:
>> On 7/22/22 07:00, Takahiro Kuwano wrote:
>>
>> Good morning, Takahiro!
>>
> Good morning, Tudor!
>
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 7/22/2022 1:06 AM, Tudor.Ambarus@microchip.com wrote:
>>>> On 5/23/22 10:49, Michael Walle wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> Am 2022-05-14 05:51, schrieb Takahiro Kuwano:
>>>>>> On 5/13/2022 6:40 PM, Michael Walle wrote:
>>>>>>> [btw the subject still has the old name of the addr_width]
>>>>>>>
>>>>>> Yes, it must be fixed in next rev.
>>>>>>
>>>>>>> Am 2022-05-13 03:26, schrieb Takahiro Kuwano:
>>>>>>>> On 5/13/2022 7:14 AM, Michael Walle wrote:
>>>>>>>>> Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
>>>>>>>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>>>>>>>
>>>>>>>>>> In 4BAIT parse, keep nor->params->addr_width because it may be used
>>>>>>>>>> as
>>>>>>>>>> current address mode in SMPT parse later on.
>>>>>>>>>
>>>>>>>>> Mh I'm not sure this is needed at all.
>>>>>>>>>
>>>>>>>>> SFDP spec says
>>>>>>>>> Variable address length (the current setting of the address
>>>>>>>>> length mode defines the address length)
>>>>>>>>>
>>>>>>>>> and
>>>>>>>>> When the length is defined as variable, the software or hardware
>>>>>>>>> controlling the memory is aware of the address length mode last
>>>>>>>>> set in the memory device and this same length of address.
>>>>>>>>>
>>>>>>>>> We don't set any address mode until all the SFDP parsing is
>>>>>>>>> over. Therefore we should always be in 3 byte mode, no?
>>>>>>>>>
>>>>>>>> Actually there are some devices that have variable address length but
>>>>>>>> 4 byte mode by default (I will work on those devices after this
>>>>>>>> series
>>>>>>>> is settled). To support such case, I prefer to use
>>>>>>>> params->addr_nbytes
>>>>>>>> as current address mode so that I can fix it in post_bfpt_fixup()
>>>>>>>> hook.
>>>>>>>
>>>>>>> Are there public datasheets available? So these devices have a 3 byte
>>>>>> I will send datasheets to you in another email. At this point, only
>>>>>> summary datasheet is available in website.
>>>>>>
>>>>>>> and a 4 byte mode, but after reset, they are in the 4 byte mode? Looks
>>>>>> Yes.
>>>>>>
>>>>>>> like it should be fixed in a different way. I'm not sure the "current
>>>>>>> mode" handling is correct.
>>>>>>>
>>>>>> Yes, we may want to introduce a new flag like SPI_NOR_4BAM_DEFAULT and
>>>>>> check
>>>>>> the flag in BFPT parse. Once I send another series, please review.
>>>>>>
>>>>>>> We need to differentiate between the mode the flash currently is using
>>>>>>> (nor->addr_nbytes) and the mode parsed by SFDP (params->addr_nbytes).
>>>>>>>
>>>>>> The flash's address mode affects the address length of Non-4B opcodes,
>>>>>> including read/write any register ops used in SMPT parse and Infineon
>>>>>> (spansion) specific hooks.
>>>>>>
>>>>>> The 4B opcodes always take address length of 4 regardless of flash's
>>>>>> address mode. In these Infineon chips, 4B opcodes for read/program/
>>>>>> erase are available and 4BAIT advertises them. We don't have to enter
>>>>>> 4 byte address mode for read/program/erase.
>>>>>
>>>>> btw. this is a pity. you are using the stateless 4b opcodes but
>>>>> then you don't provide stateless opcodes for the read any register
>>>>> op :/
>>>>>
>>>>>> So, I think we need to differentiate between address length for
>>>>>> read/program/erase and flash's default address mode.
>>>>>
>>>>> Or we keep them in sync. E.g. switch to 4bytes mode if we are
>>>>> using the 4 byte. Granted, that sounds like a hack :)
>>>>>
>>>>>> Obviously we are using nor->addr_nbytes as address length for read/
>>>>>> program/erase and should keep this usage.
>>>>>
>>>>> Yes, I wasn't aware that we actually two different runtime
>>>>> parameters:
>>>>> - the read/program/erase address width, also used with the
>>>>> 4b opcodes
>>>>> - internal mode 3b/4b. Up until now, this wasn't an issue
>>>>> because either the mode was switched or the 4b opcodes
>>>>> were used. So this was mutually exclusive. Now we have
>>>>> flashes which uses 4b opcodes _and_ we need the state
>>>>> of the internal mode.
>>>>>
>>>>> I can't think of a good solution for now. Need to think
>>>>> more about this, but I'm pretty busy at the moment.
>>>>> What I think is clear is that we need two different modes
>>>>> here in the spi_nor struct. nor->addr_nbytes for the
>>>>> read/program/erase opcodes and nor->address_mode or similar
>>>>> which tracks the SPI flash's internal address mode.
>>>>
>>>> Hi, Takahiro,
>>>>
>>>> Can we determine the flash's internal address mode by querying
>>>> the flash at run-time? Is this possible on Semper flashes?
>>>>
>>> CFR2V[7] has current address mode, but to read that, we need to
>>> issue Read Any Register which address length relies on current
>>> address mode. Chicken-and-egg...
>>>
>>
>> I see. What happens if we issue the Read Any Register command with
>> the wrong address internal mode? Will I read just 0xff?
>> For example try reading CFR2V[7] using addr_nbytes of value 3, and
>> if it fails, to read it again but this time using addr_nbytes of
>> value 4.
>>
> It's undetermined. In case we issue Read Any Register with 3B address,
> the host controller outputs 4 bytes (opcode + address) then inputs
> 1 byte. If the device is in 4B address mode at this time, the host
> controller inputs the 1 byte while device is still waits for LSB of
> address so IO is not driven by device.
>
So if the IO lines are floating, we'll "read" whatever is indicated
by the IOs at that specific moment of time, right?
If so, we're forced to statically specify the default internal address
mode.
--
Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
2022-07-22 5:06 ` Tudor.Ambarus
@ 2022-07-22 5:11 ` Takahiro Kuwano
2022-07-22 6:08 ` Tudor.Ambarus
0 siblings, 1 reply; 42+ messages in thread
From: Takahiro Kuwano @ 2022-07-22 5:11 UTC (permalink / raw)
To: Tudor.Ambarus, michael
Cc: linux-mtd, miquel.raynal, richard, vigneshr, p.yadav,
Bacem.Daassi, Takahiro.Kuwano
On 7/22/2022 2:06 PM, Tudor.Ambarus@microchip.com wrote:
> On 7/22/22 07:46, Takahiro Kuwano wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 7/22/2022 1:20 PM, Tudor.Ambarus@microchip.com wrote:
>>> On 7/22/22 07:00, Takahiro Kuwano wrote:
>>>
>>> Good morning, Takahiro!
>>>
>> Good morning, Tudor!
>>
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> On 7/22/2022 1:06 AM, Tudor.Ambarus@microchip.com wrote:
>>>>> On 5/23/22 10:49, Michael Walle wrote:
>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>
>>>>>> Am 2022-05-14 05:51, schrieb Takahiro Kuwano:
>>>>>>> On 5/13/2022 6:40 PM, Michael Walle wrote:
>>>>>>>> [btw the subject still has the old name of the addr_width]
>>>>>>>>
>>>>>>> Yes, it must be fixed in next rev.
>>>>>>>
>>>>>>>> Am 2022-05-13 03:26, schrieb Takahiro Kuwano:
>>>>>>>>> On 5/13/2022 7:14 AM, Michael Walle wrote:
>>>>>>>>>> Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
>>>>>>>>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>>>>>>>>
>>>>>>>>>>> In 4BAIT parse, keep nor->params->addr_width because it may be used
>>>>>>>>>>> as
>>>>>>>>>>> current address mode in SMPT parse later on.
>>>>>>>>>>
>>>>>>>>>> Mh I'm not sure this is needed at all.
>>>>>>>>>>
>>>>>>>>>> SFDP spec says
>>>>>>>>>> Variable address length (the current setting of the address
>>>>>>>>>> length mode defines the address length)
>>>>>>>>>>
>>>>>>>>>> and
>>>>>>>>>> When the length is defined as variable, the software or hardware
>>>>>>>>>> controlling the memory is aware of the address length mode last
>>>>>>>>>> set in the memory device and this same length of address.
>>>>>>>>>>
>>>>>>>>>> We don't set any address mode until all the SFDP parsing is
>>>>>>>>>> over. Therefore we should always be in 3 byte mode, no?
>>>>>>>>>>
>>>>>>>>> Actually there are some devices that have variable address length but
>>>>>>>>> 4 byte mode by default (I will work on those devices after this
>>>>>>>>> series
>>>>>>>>> is settled). To support such case, I prefer to use
>>>>>>>>> params->addr_nbytes
>>>>>>>>> as current address mode so that I can fix it in post_bfpt_fixup()
>>>>>>>>> hook.
>>>>>>>>
>>>>>>>> Are there public datasheets available? So these devices have a 3 byte
>>>>>>> I will send datasheets to you in another email. At this point, only
>>>>>>> summary datasheet is available in website.
>>>>>>>
>>>>>>>> and a 4 byte mode, but after reset, they are in the 4 byte mode? Looks
>>>>>>> Yes.
>>>>>>>
>>>>>>>> like it should be fixed in a different way. I'm not sure the "current
>>>>>>>> mode" handling is correct.
>>>>>>>>
>>>>>>> Yes, we may want to introduce a new flag like SPI_NOR_4BAM_DEFAULT and
>>>>>>> check
>>>>>>> the flag in BFPT parse. Once I send another series, please review.
>>>>>>>
>>>>>>>> We need to differentiate between the mode the flash currently is using
>>>>>>>> (nor->addr_nbytes) and the mode parsed by SFDP (params->addr_nbytes).
>>>>>>>>
>>>>>>> The flash's address mode affects the address length of Non-4B opcodes,
>>>>>>> including read/write any register ops used in SMPT parse and Infineon
>>>>>>> (spansion) specific hooks.
>>>>>>>
>>>>>>> The 4B opcodes always take address length of 4 regardless of flash's
>>>>>>> address mode. In these Infineon chips, 4B opcodes for read/program/
>>>>>>> erase are available and 4BAIT advertises them. We don't have to enter
>>>>>>> 4 byte address mode for read/program/erase.
>>>>>>
>>>>>> btw. this is a pity. you are using the stateless 4b opcodes but
>>>>>> then you don't provide stateless opcodes for the read any register
>>>>>> op :/
>>>>>>
>>>>>>> So, I think we need to differentiate between address length for
>>>>>>> read/program/erase and flash's default address mode.
>>>>>>
>>>>>> Or we keep them in sync. E.g. switch to 4bytes mode if we are
>>>>>> using the 4 byte. Granted, that sounds like a hack :)
>>>>>>
>>>>>>> Obviously we are using nor->addr_nbytes as address length for read/
>>>>>>> program/erase and should keep this usage.
>>>>>>
>>>>>> Yes, I wasn't aware that we actually two different runtime
>>>>>> parameters:
>>>>>> - the read/program/erase address width, also used with the
>>>>>> 4b opcodes
>>>>>> - internal mode 3b/4b. Up until now, this wasn't an issue
>>>>>> because either the mode was switched or the 4b opcodes
>>>>>> were used. So this was mutually exclusive. Now we have
>>>>>> flashes which uses 4b opcodes _and_ we need the state
>>>>>> of the internal mode.
>>>>>>
>>>>>> I can't think of a good solution for now. Need to think
>>>>>> more about this, but I'm pretty busy at the moment.
>>>>>> What I think is clear is that we need two different modes
>>>>>> here in the spi_nor struct. nor->addr_nbytes for the
>>>>>> read/program/erase opcodes and nor->address_mode or similar
>>>>>> which tracks the SPI flash's internal address mode.
>>>>>
>>>>> Hi, Takahiro,
>>>>>
>>>>> Can we determine the flash's internal address mode by querying
>>>>> the flash at run-time? Is this possible on Semper flashes?
>>>>>
>>>> CFR2V[7] has current address mode, but to read that, we need to
>>>> issue Read Any Register which address length relies on current
>>>> address mode. Chicken-and-egg...
>>>>
>>>
>>> I see. What happens if we issue the Read Any Register command with
>>> the wrong address internal mode? Will I read just 0xff?
>>> For example try reading CFR2V[7] using addr_nbytes of value 3, and
>>> if it fails, to read it again but this time using addr_nbytes of
>>> value 4.
>>>
>> It's undetermined. In case we issue Read Any Register with 3B address,
>> the host controller outputs 4 bytes (opcode + address) then inputs
>> 1 byte. If the device is in 4B address mode at this time, the host
>> controller inputs the 1 byte while device is still waits for LSB of
>> address so IO is not driven by device.
>>
>
> So if the IO lines are floating, we'll "read" whatever is indicated
> by the IOs at that specific moment of time, right?
>
Yes, right.
> If so, we're forced to statically specify the default internal address
> mode.
>
Yes.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
2022-07-22 5:11 ` Takahiro Kuwano
@ 2022-07-22 6:08 ` Tudor.Ambarus
2022-07-22 7:38 ` Tudor.Ambarus
0 siblings, 1 reply; 42+ messages in thread
From: Tudor.Ambarus @ 2022-07-22 6:08 UTC (permalink / raw)
To: tkuw584924, michael
Cc: linux-mtd, miquel.raynal, richard, vigneshr, p.yadav,
Bacem.Daassi, Takahiro.Kuwano
On 7/22/22 08:11, Takahiro Kuwano wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 7/22/2022 2:06 PM, Tudor.Ambarus@microchip.com wrote:
>> On 7/22/22 07:46, Takahiro Kuwano wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 7/22/2022 1:20 PM, Tudor.Ambarus@microchip.com wrote:
>>>> On 7/22/22 07:00, Takahiro Kuwano wrote:
>>>>
>>>> Good morning, Takahiro!
>>>>
>>> Good morning, Tudor!
>>>
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> On 7/22/2022 1:06 AM, Tudor.Ambarus@microchip.com wrote:
>>>>>> On 5/23/22 10:49, Michael Walle wrote:
>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>>
>>>>>>> Am 2022-05-14 05:51, schrieb Takahiro Kuwano:
>>>>>>>> On 5/13/2022 6:40 PM, Michael Walle wrote:
>>>>>>>>> [btw the subject still has the old name of the addr_width]
>>>>>>>>>
>>>>>>>> Yes, it must be fixed in next rev.
>>>>>>>>
>>>>>>>>> Am 2022-05-13 03:26, schrieb Takahiro Kuwano:
>>>>>>>>>> On 5/13/2022 7:14 AM, Michael Walle wrote:
>>>>>>>>>>> Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
>>>>>>>>>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>>>>>>>>>
>>>>>>>>>>>> In 4BAIT parse, keep nor->params->addr_width because it may be used
>>>>>>>>>>>> as
>>>>>>>>>>>> current address mode in SMPT parse later on.
>>>>>>>>>>>
>>>>>>>>>>> Mh I'm not sure this is needed at all.
>>>>>>>>>>>
>>>>>>>>>>> SFDP spec says
>>>>>>>>>>> Variable address length (the current setting of the address
>>>>>>>>>>> length mode defines the address length)
>>>>>>>>>>>
>>>>>>>>>>> and
>>>>>>>>>>> When the length is defined as variable, the software or hardware
>>>>>>>>>>> controlling the memory is aware of the address length mode last
>>>>>>>>>>> set in the memory device and this same length of address.
>>>>>>>>>>>
>>>>>>>>>>> We don't set any address mode until all the SFDP parsing is
>>>>>>>>>>> over. Therefore we should always be in 3 byte mode, no?
>>>>>>>>>>>
>>>>>>>>>> Actually there are some devices that have variable address length but
>>>>>>>>>> 4 byte mode by default (I will work on those devices after this
>>>>>>>>>> series
>>>>>>>>>> is settled). To support such case, I prefer to use
>>>>>>>>>> params->addr_nbytes
>>>>>>>>>> as current address mode so that I can fix it in post_bfpt_fixup()
>>>>>>>>>> hook.
>>>>>>>>>
>>>>>>>>> Are there public datasheets available? So these devices have a 3 byte
>>>>>>>> I will send datasheets to you in another email. At this point, only
>>>>>>>> summary datasheet is available in website.
>>>>>>>>
>>>>>>>>> and a 4 byte mode, but after reset, they are in the 4 byte mode? Looks
>>>>>>>> Yes.
>>>>>>>>
>>>>>>>>> like it should be fixed in a different way. I'm not sure the "current
>>>>>>>>> mode" handling is correct.
>>>>>>>>>
>>>>>>>> Yes, we may want to introduce a new flag like SPI_NOR_4BAM_DEFAULT and
>>>>>>>> check
>>>>>>>> the flag in BFPT parse. Once I send another series, please review.
>>>>>>>>
>>>>>>>>> We need to differentiate between the mode the flash currently is using
>>>>>>>>> (nor->addr_nbytes) and the mode parsed by SFDP (params->addr_nbytes).
>>>>>>>>>
>>>>>>>> The flash's address mode affects the address length of Non-4B opcodes,
>>>>>>>> including read/write any register ops used in SMPT parse and Infineon
>>>>>>>> (spansion) specific hooks.
>>>>>>>>
>>>>>>>> The 4B opcodes always take address length of 4 regardless of flash's
>>>>>>>> address mode. In these Infineon chips, 4B opcodes for read/program/
>>>>>>>> erase are available and 4BAIT advertises them. We don't have to enter
>>>>>>>> 4 byte address mode for read/program/erase.
>>>>>>>
>>>>>>> btw. this is a pity. you are using the stateless 4b opcodes but
>>>>>>> then you don't provide stateless opcodes for the read any register
>>>>>>> op :/
>>>>>>>
>>>>>>>> So, I think we need to differentiate between address length for
>>>>>>>> read/program/erase and flash's default address mode.
>>>>>>>
>>>>>>> Or we keep them in sync. E.g. switch to 4bytes mode if we are
>>>>>>> using the 4 byte. Granted, that sounds like a hack :)
>>>>>>>
>>>>>>>> Obviously we are using nor->addr_nbytes as address length for read/
>>>>>>>> program/erase and should keep this usage.
>>>>>>>
>>>>>>> Yes, I wasn't aware that we actually two different runtime
>>>>>>> parameters:
>>>>>>> - the read/program/erase address width, also used with the
>>>>>>> 4b opcodes
>>>>>>> - internal mode 3b/4b. Up until now, this wasn't an issue
>>>>>>> because either the mode was switched or the 4b opcodes
>>>>>>> were used. So this was mutually exclusive. Now we have
>>>>>>> flashes which uses 4b opcodes _and_ we need the state
>>>>>>> of the internal mode.
>>>>>>>
>>>>>>> I can't think of a good solution for now. Need to think
>>>>>>> more about this, but I'm pretty busy at the moment.
>>>>>>> What I think is clear is that we need two different modes
>>>>>>> here in the spi_nor struct. nor->addr_nbytes for the
>>>>>>> read/program/erase opcodes and nor->address_mode or similar
>>>>>>> which tracks the SPI flash's internal address mode.
>>>>>>
>>>>>> Hi, Takahiro,
>>>>>>
>>>>>> Can we determine the flash's internal address mode by querying
>>>>>> the flash at run-time? Is this possible on Semper flashes?
>>>>>>
>>>>> CFR2V[7] has current address mode, but to read that, we need to
>>>>> issue Read Any Register which address length relies on current
>>>>> address mode. Chicken-and-egg...
>>>>>
>>>>
>>>> I see. What happens if we issue the Read Any Register command with
>>>> the wrong address internal mode? Will I read just 0xff?
>>>> For example try reading CFR2V[7] using addr_nbytes of value 3, and
>>>> if it fails, to read it again but this time using addr_nbytes of
>>>> value 4.
>>>>
>>> It's undetermined. In case we issue Read Any Register with 3B address,
>>> the host controller outputs 4 bytes (opcode + address) then inputs
>>> 1 byte. If the device is in 4B address mode at this time, the host
>>> controller inputs the 1 byte while device is still waits for LSB of
>>> address so IO is not driven by device.
>>>
>>
>> So if the IO lines are floating, we'll "read" whatever is indicated
>> by the IOs at that specific moment of time, right?
>>
> Yes, right.
>
>> If so, we're forced to statically specify the default internal address
>> mode.
>>
> Yes.
OK, let me scratch something.
--
Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
2022-07-22 6:08 ` Tudor.Ambarus
@ 2022-07-22 7:38 ` Tudor.Ambarus
2022-07-22 7:45 ` Tudor.Ambarus
0 siblings, 1 reply; 42+ messages in thread
From: Tudor.Ambarus @ 2022-07-22 7:38 UTC (permalink / raw)
To: tkuw584924, michael
Cc: linux-mtd, miquel.raynal, richard, vigneshr, p.yadav,
Bacem.Daassi, Takahiro.Kuwano
On 7/22/22 09:08, Tudor.Ambarus@microchip.com wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 7/22/22 08:11, Takahiro Kuwano wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 7/22/2022 2:06 PM, Tudor.Ambarus@microchip.com wrote:
>>> On 7/22/22 07:46, Takahiro Kuwano wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> On 7/22/2022 1:20 PM, Tudor.Ambarus@microchip.com wrote:
>>>>> On 7/22/22 07:00, Takahiro Kuwano wrote:
>>>>>
>>>>> Good morning, Takahiro!
>>>>>
>>>> Good morning, Tudor!
>>>>
>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>
>>>>>> On 7/22/2022 1:06 AM, Tudor.Ambarus@microchip.com wrote:
>>>>>>> On 5/23/22 10:49, Michael Walle wrote:
>>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>>>
>>>>>>>> Am 2022-05-14 05:51, schrieb Takahiro Kuwano:
>>>>>>>>> On 5/13/2022 6:40 PM, Michael Walle wrote:
>>>>>>>>>> [btw the subject still has the old name of the addr_width]
>>>>>>>>>>
>>>>>>>>> Yes, it must be fixed in next rev.
>>>>>>>>>
>>>>>>>>>> Am 2022-05-13 03:26, schrieb Takahiro Kuwano:
>>>>>>>>>>> On 5/13/2022 7:14 AM, Michael Walle wrote:
>>>>>>>>>>>> Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
>>>>>>>>>>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>>>>>>>>>>
>>>>>>>>>>>>> In 4BAIT parse, keep nor->params->addr_width because it may be used
>>>>>>>>>>>>> as
>>>>>>>>>>>>> current address mode in SMPT parse later on.
>>>>>>>>>>>>
>>>>>>>>>>>> Mh I'm not sure this is needed at all.
>>>>>>>>>>>>
>>>>>>>>>>>> SFDP spec says
>>>>>>>>>>>> Variable address length (the current setting of the address
>>>>>>>>>>>> length mode defines the address length)
>>>>>>>>>>>>
>>>>>>>>>>>> and
>>>>>>>>>>>> When the length is defined as variable, the software or hardware
>>>>>>>>>>>> controlling the memory is aware of the address length mode last
>>>>>>>>>>>> set in the memory device and this same length of address.
>>>>>>>>>>>>
>>>>>>>>>>>> We don't set any address mode until all the SFDP parsing is
>>>>>>>>>>>> over. Therefore we should always be in 3 byte mode, no?
>>>>>>>>>>>>
>>>>>>>>>>> Actually there are some devices that have variable address length but
>>>>>>>>>>> 4 byte mode by default (I will work on those devices after this
>>>>>>>>>>> series
>>>>>>>>>>> is settled). To support such case, I prefer to use
>>>>>>>>>>> params->addr_nbytes
>>>>>>>>>>> as current address mode so that I can fix it in post_bfpt_fixup()
>>>>>>>>>>> hook.
>>>>>>>>>>
>>>>>>>>>> Are there public datasheets available? So these devices have a 3 byte
>>>>>>>>> I will send datasheets to you in another email. At this point, only
>>>>>>>>> summary datasheet is available in website.
>>>>>>>>>
>>>>>>>>>> and a 4 byte mode, but after reset, they are in the 4 byte mode? Looks
>>>>>>>>> Yes.
>>>>>>>>>
>>>>>>>>>> like it should be fixed in a different way. I'm not sure the "current
>>>>>>>>>> mode" handling is correct.
>>>>>>>>>>
>>>>>>>>> Yes, we may want to introduce a new flag like SPI_NOR_4BAM_DEFAULT and
>>>>>>>>> check
>>>>>>>>> the flag in BFPT parse. Once I send another series, please review.
>>>>>>>>>
>>>>>>>>>> We need to differentiate between the mode the flash currently is using
>>>>>>>>>> (nor->addr_nbytes) and the mode parsed by SFDP (params->addr_nbytes).
>>>>>>>>>>
>>>>>>>>> The flash's address mode affects the address length of Non-4B opcodes,
>>>>>>>>> including read/write any register ops used in SMPT parse and Infineon
>>>>>>>>> (spansion) specific hooks.
>>>>>>>>>
>>>>>>>>> The 4B opcodes always take address length of 4 regardless of flash's
>>>>>>>>> address mode. In these Infineon chips, 4B opcodes for read/program/
>>>>>>>>> erase are available and 4BAIT advertises them. We don't have to enter
>>>>>>>>> 4 byte address mode for read/program/erase.
>>>>>>>>
>>>>>>>> btw. this is a pity. you are using the stateless 4b opcodes but
>>>>>>>> then you don't provide stateless opcodes for the read any register
>>>>>>>> op :/
>>>>>>>>
>>>>>>>>> So, I think we need to differentiate between address length for
>>>>>>>>> read/program/erase and flash's default address mode.
>>>>>>>>
>>>>>>>> Or we keep them in sync. E.g. switch to 4bytes mode if we are
>>>>>>>> using the 4 byte. Granted, that sounds like a hack :)
>>>>>>>>
>>>>>>>>> Obviously we are using nor->addr_nbytes as address length for read/
>>>>>>>>> program/erase and should keep this usage.
>>>>>>>>
>>>>>>>> Yes, I wasn't aware that we actually two different runtime
>>>>>>>> parameters:
>>>>>>>> - the read/program/erase address width, also used with the
>>>>>>>> 4b opcodes
>>>>>>>> - internal mode 3b/4b. Up until now, this wasn't an issue
>>>>>>>> because either the mode was switched or the 4b opcodes
>>>>>>>> were used. So this was mutually exclusive. Now we have
>>>>>>>> flashes which uses 4b opcodes _and_ we need the state
>>>>>>>> of the internal mode.
>>>>>>>>
>>>>>>>> I can't think of a good solution for now. Need to think
>>>>>>>> more about this, but I'm pretty busy at the moment.
>>>>>>>> What I think is clear is that we need two different modes
>>>>>>>> here in the spi_nor struct. nor->addr_nbytes for the
>>>>>>>> read/program/erase opcodes and nor->address_mode or similar
>>>>>>>> which tracks the SPI flash's internal address mode.
>>>>>>>
>>>>>>> Hi, Takahiro,
>>>>>>>
>>>>>>> Can we determine the flash's internal address mode by querying
>>>>>>> the flash at run-time? Is this possible on Semper flashes?
>>>>>>>
>>>>>> CFR2V[7] has current address mode, but to read that, we need to
>>>>>> issue Read Any Register which address length relies on current
>>>>>> address mode. Chicken-and-egg...
>>>>>>
>>>>>
>>>>> I see. What happens if we issue the Read Any Register command with
>>>>> the wrong address internal mode? Will I read just 0xff?
>>>>> For example try reading CFR2V[7] using addr_nbytes of value 3, and
>>>>> if it fails, to read it again but this time using addr_nbytes of
>>>>> value 4.
>>>>>
>>>> It's undetermined. In case we issue Read Any Register with 3B address,
>>>> the host controller outputs 4 bytes (opcode + address) then inputs
>>>> 1 byte. If the device is in 4B address mode at this time, the host
>>>> controller inputs the 1 byte while device is still waits for LSB of
>>>> address so IO is not driven by device.
>>>>
>>>
>>> So if the IO lines are floating, we'll "read" whatever is indicated
>>> by the IOs at that specific moment of time, right?
>>>
>> Yes, right.
>>
>>> If so, we're forced to statically specify the default internal address
>>> mode.
>>>
>> Yes.
> OK, let me scratch something.
>
Would you please send me the datasheet for Semper flashes?
I noticed that the addr mode is controlled via a volatile conf register
in your case, but there are flashes like w25q256jv that control the
addr mode via a non-volatile register, so only a default value will
not empower flashes with NV register conf to actually toggle that bit.
We may introduce a dt prop for the NV case when needed, the disadvantage
being that the generic jedec,spi-nor will not be so generic, as it will
have a fixed default NV addr mode specified in dt.
--
Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
2022-07-22 7:38 ` Tudor.Ambarus
@ 2022-07-22 7:45 ` Tudor.Ambarus
0 siblings, 0 replies; 42+ messages in thread
From: Tudor.Ambarus @ 2022-07-22 7:45 UTC (permalink / raw)
To: tkuw584924, michael
Cc: linux-mtd, miquel.raynal, richard, vigneshr, p.yadav,
Bacem.Daassi, Takahiro.Kuwano
On 7/22/22 10:38, Tudor Ambarus - M18064 wrote:
> On 7/22/22 09:08, Tudor.Ambarus@microchip.com wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 7/22/22 08:11, Takahiro Kuwano wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 7/22/2022 2:06 PM, Tudor.Ambarus@microchip.com wrote:
>>>> On 7/22/22 07:46, Takahiro Kuwano wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> On 7/22/2022 1:20 PM, Tudor.Ambarus@microchip.com wrote:
>>>>>> On 7/22/22 07:00, Takahiro Kuwano wrote:
>>>>>>
>>>>>> Good morning, Takahiro!
>>>>>>
>>>>> Good morning, Tudor!
>>>>>
>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>>
>>>>>>> On 7/22/2022 1:06 AM, Tudor.Ambarus@microchip.com wrote:
>>>>>>>> On 5/23/22 10:49, Michael Walle wrote:
>>>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>>>>
>>>>>>>>> Am 2022-05-14 05:51, schrieb Takahiro Kuwano:
>>>>>>>>>> On 5/13/2022 6:40 PM, Michael Walle wrote:
>>>>>>>>>>> [btw the subject still has the old name of the addr_width]
>>>>>>>>>>>
>>>>>>>>>> Yes, it must be fixed in next rev.
>>>>>>>>>>
>>>>>>>>>>> Am 2022-05-13 03:26, schrieb Takahiro Kuwano:
>>>>>>>>>>>> On 5/13/2022 7:14 AM, Michael Walle wrote:
>>>>>>>>>>>>> Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
>>>>>>>>>>>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> In 4BAIT parse, keep nor->params->addr_width because it may be used
>>>>>>>>>>>>>> as
>>>>>>>>>>>>>> current address mode in SMPT parse later on.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Mh I'm not sure this is needed at all.
>>>>>>>>>>>>>
>>>>>>>>>>>>> SFDP spec says
>>>>>>>>>>>>> Variable address length (the current setting of the address
>>>>>>>>>>>>> length mode defines the address length)
>>>>>>>>>>>>>
>>>>>>>>>>>>> and
>>>>>>>>>>>>> When the length is defined as variable, the software or hardware
>>>>>>>>>>>>> controlling the memory is aware of the address length mode last
>>>>>>>>>>>>> set in the memory device and this same length of address.
>>>>>>>>>>>>>
>>>>>>>>>>>>> We don't set any address mode until all the SFDP parsing is
>>>>>>>>>>>>> over. Therefore we should always be in 3 byte mode, no?
>>>>>>>>>>>>>
>>>>>>>>>>>> Actually there are some devices that have variable address length but
>>>>>>>>>>>> 4 byte mode by default (I will work on those devices after this
>>>>>>>>>>>> series
>>>>>>>>>>>> is settled). To support such case, I prefer to use
>>>>>>>>>>>> params->addr_nbytes
>>>>>>>>>>>> as current address mode so that I can fix it in post_bfpt_fixup()
>>>>>>>>>>>> hook.
>>>>>>>>>>>
>>>>>>>>>>> Are there public datasheets available? So these devices have a 3 byte
>>>>>>>>>> I will send datasheets to you in another email. At this point, only
>>>>>>>>>> summary datasheet is available in website.
>>>>>>>>>>
>>>>>>>>>>> and a 4 byte mode, but after reset, they are in the 4 byte mode? Looks
>>>>>>>>>> Yes.
>>>>>>>>>>
>>>>>>>>>>> like it should be fixed in a different way. I'm not sure the "current
>>>>>>>>>>> mode" handling is correct.
>>>>>>>>>>>
>>>>>>>>>> Yes, we may want to introduce a new flag like SPI_NOR_4BAM_DEFAULT and
>>>>>>>>>> check
>>>>>>>>>> the flag in BFPT parse. Once I send another series, please review.
>>>>>>>>>>
>>>>>>>>>>> We need to differentiate between the mode the flash currently is using
>>>>>>>>>>> (nor->addr_nbytes) and the mode parsed by SFDP (params->addr_nbytes).
>>>>>>>>>>>
>>>>>>>>>> The flash's address mode affects the address length of Non-4B opcodes,
>>>>>>>>>> including read/write any register ops used in SMPT parse and Infineon
>>>>>>>>>> (spansion) specific hooks.
>>>>>>>>>>
>>>>>>>>>> The 4B opcodes always take address length of 4 regardless of flash's
>>>>>>>>>> address mode. In these Infineon chips, 4B opcodes for read/program/
>>>>>>>>>> erase are available and 4BAIT advertises them. We don't have to enter
>>>>>>>>>> 4 byte address mode for read/program/erase.
>>>>>>>>>
>>>>>>>>> btw. this is a pity. you are using the stateless 4b opcodes but
>>>>>>>>> then you don't provide stateless opcodes for the read any register
>>>>>>>>> op :/
>>>>>>>>>
>>>>>>>>>> So, I think we need to differentiate between address length for
>>>>>>>>>> read/program/erase and flash's default address mode.
>>>>>>>>>
>>>>>>>>> Or we keep them in sync. E.g. switch to 4bytes mode if we are
>>>>>>>>> using the 4 byte. Granted, that sounds like a hack :)
>>>>>>>>>
>>>>>>>>>> Obviously we are using nor->addr_nbytes as address length for read/
>>>>>>>>>> program/erase and should keep this usage.
>>>>>>>>>
>>>>>>>>> Yes, I wasn't aware that we actually two different runtime
>>>>>>>>> parameters:
>>>>>>>>> - the read/program/erase address width, also used with the
>>>>>>>>> 4b opcodes
>>>>>>>>> - internal mode 3b/4b. Up until now, this wasn't an issue
>>>>>>>>> because either the mode was switched or the 4b opcodes
>>>>>>>>> were used. So this was mutually exclusive. Now we have
>>>>>>>>> flashes which uses 4b opcodes _and_ we need the state
>>>>>>>>> of the internal mode.
>>>>>>>>>
>>>>>>>>> I can't think of a good solution for now. Need to think
>>>>>>>>> more about this, but I'm pretty busy at the moment.
>>>>>>>>> What I think is clear is that we need two different modes
>>>>>>>>> here in the spi_nor struct. nor->addr_nbytes for the
>>>>>>>>> read/program/erase opcodes and nor->address_mode or similar
>>>>>>>>> which tracks the SPI flash's internal address mode.
>>>>>>>>
>>>>>>>> Hi, Takahiro,
>>>>>>>>
>>>>>>>> Can we determine the flash's internal address mode by querying
>>>>>>>> the flash at run-time? Is this possible on Semper flashes?
>>>>>>>>
>>>>>>> CFR2V[7] has current address mode, but to read that, we need to
>>>>>>> issue Read Any Register which address length relies on current
>>>>>>> address mode. Chicken-and-egg...
>>>>>>>
>>>>>>
>>>>>> I see. What happens if we issue the Read Any Register command with
>>>>>> the wrong address internal mode? Will I read just 0xff?
>>>>>> For example try reading CFR2V[7] using addr_nbytes of value 3, and
>>>>>> if it fails, to read it again but this time using addr_nbytes of
>>>>>> value 4.
>>>>>>
>>>>> It's undetermined. In case we issue Read Any Register with 3B address,
>>>>> the host controller outputs 4 bytes (opcode + address) then inputs
>>>>> 1 byte. If the device is in 4B address mode at this time, the host
>>>>> controller inputs the 1 byte while device is still waits for LSB of
>>>>> address so IO is not driven by device.
>>>>>
>>>>
>>>> So if the IO lines are floating, we'll "read" whatever is indicated
>>>> by the IOs at that specific moment of time, right?
>>>>
>>> Yes, right.
>>>
>>>> If so, we're forced to statically specify the default internal address
>>>> mode.
>>>>
>>> Yes.
>> OK, let me scratch something.
>>
>
> Would you please send me the datasheet for Semper flashes?
> I noticed that the addr mode is controlled via a volatile conf register
> in your case, but there are flashes like w25q256jv that control the
> addr mode via a non-volatile register, so only a default value will
> not empower flashes with NV register conf to actually toggle that bit.
> We may introduce a dt prop for the NV case when needed, the disadvantage
> being that the generic jedec,spi-nor will not be so generic, as it will
> have a fixed default NV addr mode specified in dt.
>
Oh, but winbond uses a dedicated RDSR3 opcode and doesn't need addr_nbytes,
so it's safe. It looks like we can use an addr_mode_nbytes after all.
--
Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
2022-05-14 3:51 ` Takahiro Kuwano
2022-05-18 6:04 ` Takahiro Kuwano
2022-05-23 7:49 ` Michael Walle
@ 2022-06-08 11:39 ` Tudor.Ambarus
2 siblings, 0 replies; 42+ messages in thread
From: Tudor.Ambarus @ 2022-06-08 11:39 UTC (permalink / raw)
To: tkuw584924, michael
Cc: linux-mtd, miquel.raynal, richard, vigneshr, p.yadav,
Bacem.Daassi, Takahiro.Kuwano
On 5/14/22 06:51, Takahiro Kuwano wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 5/13/2022 6:40 PM, Michael Walle wrote:
>> [btw the subject still has the old name of the addr_width]
>>
> Yes, it must be fixed in next rev.
>
>> Am 2022-05-13 03:26, schrieb Takahiro Kuwano:
>>> On 5/13/2022 7:14 AM, Michael Walle wrote:
>>>> Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
>>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>>
>>>>> In 4BAIT parse, keep nor->params->addr_width because it may be used as
>>>>> current address mode in SMPT parse later on.
>>>> Mh I'm not sure this is needed at all.
>>>>
>>>> SFDP spec says
>>>> Variable address length (the current setting of the address
>>>> length mode defines the address length)
>>>>
>>>> and
>>>> When the length is defined as variable, the software or hardware
>>>> controlling the memory is aware of the address length mode last
>>>> set in the memory device and this same length of address.
>>>>
>>>> We don't set any address mode until all the SFDP parsing is
>>>> over. Therefore we should always be in 3 byte mode, no?
>>>>
>>> Actually there are some devices that have variable address length but
>>> 4 byte mode by default (I will work on those devices after this series
>>> is settled). To support such case, I prefer to use params->addr_nbytes
>>> as current address mode so that I can fix it in post_bfpt_fixup() hook.
>> Are there public datasheets available? So these devices have a 3 byte
> I will send datasheets to you in another email. At this point, only
> summary datasheet is available in website.
>
>> and a 4 byte mode, but after reset, they are in the 4 byte mode? Looks
> Yes.
>
>> like it should be fixed in a different way. I'm not sure the "current
>> mode" handling is correct.
>>
> Yes, we may want to introduce a new flag like SPI_NOR_4BAM_DEFAULT and check
> the flag in BFPT parse. Once I send another series, please review.
>
Sounds good on a first look. We can consider nor->addr_nbytes of value 3 as
default and change it to 4 when such a flag is set. But you don't need such
flag in this particular case, don't you? Also, can the default number of addr
bytes be retrieved from SFDP? Anyway, let this for later and respin the series,
please.
Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v15 7/8] mtd: spi-nor: spansion: Add local function to discover page size
2022-05-09 22:10 [PATCH v15 0/8] mtd: spi-nor: Add support for Infineon s25hl-t/s25hs-t tkuw584924
` (5 preceding siblings ...)
2022-05-09 22:10 ` [PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse tkuw584924
@ 2022-05-09 22:10 ` tkuw584924
2022-05-09 22:10 ` [PATCH v15 8/8] mtd: spi-nor: spansion: Add s25hl-t/s25hs-t IDs and fixups tkuw584924
7 siblings, 0 replies; 42+ messages in thread
From: tkuw584924 @ 2022-05-09 22:10 UTC (permalink / raw)
To: linux-mtd
Cc: tudor.ambarus, miquel.raynal, richard, vigneshr, p.yadav,
michael, tkuw584924, Bacem.Daassi, Takahiro Kuwano
From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
The page size check in s28hs512t fixup can be used for s25hs/hl-t as well.
Move that to a newly created local function. Please note that the hardcoded
value 3 for the number of address bytes was replaced by
nor->params->addr_nbytes.
Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
drivers/mtd/spi-nor/spansion.c | 54 ++++++++++++++++++++--------------
1 file changed, 32 insertions(+), 22 deletions(-)
diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index 43cd6cd92537..e130f5398763 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -113,6 +113,37 @@ static int cypress_nor_octal_dtr_dis(struct spi_nor *nor)
return 0;
}
+/**
+ * cypress_nor_set_page_size() - Set page size which corresponds to the flash
+ * configuration.
+ * @nor: pointer to a 'struct spi_nor'
+ *
+ * The BFPT table advertises a 512B or 256B page size depending on part but the
+ * page size is actually configurable (with the default being 256B). Read from
+ * CFR3V[4] and set the correct size.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int cypress_nor_set_page_size(struct spi_nor *nor)
+{
+ struct spi_mem_op op =
+ CYPRESS_NOR_RD_ANY_REG_OP(nor->params->addr_nbytes,
+ SPINOR_REG_CYPRESS_CFR3V,
+ nor->bouncebuf);
+ int ret;
+
+ ret = spi_nor_read_any_reg(nor, &op, nor->reg_proto);
+ if (ret)
+ return ret;
+
+ if (nor->bouncebuf[0] & SPINOR_REG_CYPRESS_CFR3V_PGSZ)
+ nor->params->page_size = 512;
+ else
+ nor->params->page_size = 256;
+
+ return 0;
+}
+
/**
* cypress_nor_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
* @nor: pointer to a 'struct spi_nor'
@@ -167,28 +198,7 @@ static int s28hs512t_post_bfpt_fixup(struct spi_nor *nor,
const struct sfdp_parameter_header *bfpt_header,
const struct sfdp_bfpt *bfpt)
{
- /*
- * The BFPT table advertises a 512B page size but the page size is
- * actually configurable (with the default being 256B). Read from
- * CFR3V[4] and set the correct size.
- */
- struct spi_mem_op op =
- CYPRESS_NOR_RD_ANY_REG_OP(3, SPINOR_REG_CYPRESS_CFR3V,
- nor->bouncebuf);
- int ret;
-
- spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
-
- ret = spi_mem_exec_op(nor->spimem, &op);
- if (ret)
- return ret;
-
- if (nor->bouncebuf[0] & SPINOR_REG_CYPRESS_CFR3V_PGSZ)
- nor->params->page_size = 512;
- else
- nor->params->page_size = 256;
-
- return 0;
+ return cypress_nor_set_page_size(nor);
}
static const struct spi_nor_fixups s28hs512t_fixups = {
--
2.25.1
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v15 8/8] mtd: spi-nor: spansion: Add s25hl-t/s25hs-t IDs and fixups
2022-05-09 22:10 [PATCH v15 0/8] mtd: spi-nor: Add support for Infineon s25hl-t/s25hs-t tkuw584924
` (6 preceding siblings ...)
2022-05-09 22:10 ` [PATCH v15 7/8] mtd: spi-nor: spansion: Add local function to discover page size tkuw584924
@ 2022-05-09 22:10 ` tkuw584924
7 siblings, 0 replies; 42+ messages in thread
From: tkuw584924 @ 2022-05-09 22:10 UTC (permalink / raw)
To: linux-mtd
Cc: tudor.ambarus, miquel.raynal, richard, vigneshr, p.yadav,
michael, tkuw584924, Bacem.Daassi, Takahiro Kuwano
From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
The S25HL-T/S25HS-T family is the Infineon SEMPER Flash with Quad SPI.
These Infineon chips support volatile version of configuration registers
and it is recommended to update volatile registers in the field application
due to a risk of the non-volatile registers corruption by power interrupt.
Add support for volatile QE bit.
For the single-die package parts (512Mb and 1Gb), only bottom 4KB and
uniform sector sizes are supported. This is due to missing or incorrect
entries in SMPT. Fixup for other sector sizes configurations will be
followed up as needed.
Tested on Xilinx Zynq-7000 FPGA board.
Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
drivers/mtd/spi-nor/spansion.c | 129 +++++++++++++++++++++++++++++++++
1 file changed, 129 insertions(+)
diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index e130f5398763..71c2fb1dc101 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -14,6 +14,8 @@
#define SPINOR_OP_CLSR 0x30 /* Clear status register 1 */
#define SPINOR_OP_RD_ANY_REG 0x65 /* Read any register */
#define SPINOR_OP_WR_ANY_REG 0x71 /* Write any register */
+#define SPINOR_REG_CYPRESS_CFR1V 0x00800002
+#define SPINOR_REG_CYPRESS_CFR1V_QUAD_EN BIT(1) /* Quad Enable */
#define SPINOR_REG_CYPRESS_CFR2V 0x00800003
#define SPINOR_REG_CYPRESS_CFR2V_MEMLAT_11_24 0xb
#define SPINOR_REG_CYPRESS_CFR3V 0x00800004
@@ -113,6 +115,67 @@ static int cypress_nor_octal_dtr_dis(struct spi_nor *nor)
return 0;
}
+/**
+ * cypress_nor_quad_enable_volatile() - enable Quad I/O mode in volatile
+ * register.
+ * @nor: pointer to a 'struct spi_nor'
+ *
+ * It is recommended to update volatile registers in the field application due
+ * to a risk of the non-volatile registers corruption by power interrupt. This
+ * function sets Quad Enable bit in CFR1 volatile. If users set the Quad Enable
+ * bit in the CFR1 non-volatile in advance (typically by a Flash programmer
+ * before mounting Flash on PCB), the Quad Enable bit in the CFR1 volatile is
+ * also set during Flash power-up.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int cypress_nor_quad_enable_volatile(struct spi_nor *nor)
+{
+ struct spi_mem_op op;
+ u8 cfr1v_written;
+ int ret;
+
+ op = (struct spi_mem_op)
+ CYPRESS_NOR_RD_ANY_REG_OP(nor->params->addr_nbytes,
+ SPINOR_REG_CYPRESS_CFR1V,
+ nor->bouncebuf);
+
+ ret = spi_nor_read_any_reg(nor, &op, nor->reg_proto);
+ if (ret)
+ return ret;
+
+ if (nor->bouncebuf[0] & SPINOR_REG_CYPRESS_CFR1V_QUAD_EN)
+ return 0;
+
+ /* Update the Quad Enable bit. */
+ nor->bouncebuf[0] |= SPINOR_REG_CYPRESS_CFR1V_QUAD_EN;
+ op = (struct spi_mem_op)
+ CYPRESS_NOR_WR_ANY_REG_OP(nor->params->addr_nbytes,
+ SPINOR_REG_CYPRESS_CFR1V, 1,
+ nor->bouncebuf);
+ ret = spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto);
+ if (ret)
+ return ret;
+
+ cfr1v_written = nor->bouncebuf[0];
+
+ /* Read back and check it. */
+ op = (struct spi_mem_op)
+ CYPRESS_NOR_RD_ANY_REG_OP(nor->params->addr_nbytes,
+ SPINOR_REG_CYPRESS_CFR1V,
+ nor->bouncebuf);
+ ret = spi_nor_read_any_reg(nor, &op, nor->reg_proto);
+ if (ret)
+ return ret;
+
+ if (nor->bouncebuf[0] != cfr1v_written) {
+ dev_err(nor->dev, "CFR1: Read back test failed\n");
+ return -EIO;
+ }
+
+ return 0;
+}
+
/**
* cypress_nor_set_page_size() - Set page size which corresponds to the flash
* configuration.
@@ -144,6 +207,56 @@ static int cypress_nor_set_page_size(struct spi_nor *nor)
return 0;
}
+static int
+s25hx_t_post_bfpt_fixup(struct spi_nor *nor,
+ const struct sfdp_parameter_header *bfpt_header,
+ const struct sfdp_bfpt *bfpt)
+{
+ /* Replace Quad Enable with volatile version */
+ nor->params->quad_enable = cypress_nor_quad_enable_volatile;
+
+ return cypress_nor_set_page_size(nor);
+}
+
+static void s25hx_t_post_sfdp_fixup(struct spi_nor *nor)
+{
+ struct spi_nor_erase_type *erase_type =
+ nor->params->erase_map.erase_type;
+ int i;
+
+ /*
+ * In some parts, 3byte erase opcodes are advertised by 4BAIT.
+ * Convert them to 4byte erase opcodes.
+ */
+ for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++) {
+ switch (erase_type[i].opcode) {
+ case SPINOR_OP_SE:
+ erase_type[i].opcode = SPINOR_OP_SE_4B;
+ break;
+ case SPINOR_OP_BE_4K:
+ erase_type[i].opcode = SPINOR_OP_BE_4K_4B;
+ break;
+ default:
+ break;
+ }
+ }
+}
+
+static void s25hx_t_late_init(struct spi_nor *nor)
+{
+ /* Fast Read 4B requires mode cycles */
+ nor->params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
+
+ /* The writesize should be ECC data unit size */
+ nor->params->writesize = 16;
+}
+
+static struct spi_nor_fixups s25hx_t_fixups = {
+ .post_bfpt = s25hx_t_post_bfpt_fixup,
+ .post_sfdp = s25hx_t_post_sfdp_fixup,
+ .late_init = s25hx_t_late_init,
+};
+
/**
* cypress_nor_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
* @nor: pointer to a 'struct spi_nor'
@@ -320,6 +433,22 @@ static const struct flash_info spansion_nor_parts[] = {
{ "s25fl256l", INFO(0x016019, 0, 64 * 1024, 512)
NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
FIXUP_FLAGS(SPI_NOR_4B_OPCODES) },
+ { "s25hl512t", INFO6(0x342a1a, 0x0f0390, 256 * 1024, 256)
+ PARSE_SFDP
+ MFR_FLAGS(USE_CLSR)
+ .fixups = &s25hx_t_fixups },
+ { "s25hl01gt", INFO6(0x342a1b, 0x0f0390, 256 * 1024, 512)
+ PARSE_SFDP
+ MFR_FLAGS(USE_CLSR)
+ .fixups = &s25hx_t_fixups },
+ { "s25hs512t", INFO6(0x342b1a, 0x0f0390, 256 * 1024, 256)
+ PARSE_SFDP
+ MFR_FLAGS(USE_CLSR)
+ .fixups = &s25hx_t_fixups },
+ { "s25hs01gt", INFO6(0x342b1b, 0x0f0390, 256 * 1024, 512)
+ PARSE_SFDP
+ MFR_FLAGS(USE_CLSR)
+ .fixups = &s25hx_t_fixups },
{ "cy15x104q", INFO6(0x042cc2, 0x7f7f7f, 512 * 1024, 1)
FLAGS(SPI_NOR_NO_ERASE) },
{ "s28hs512t", INFO(0x345b1a, 0, 256 * 1024, 256)
--
2.25.1
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 42+ messages in thread