This is a combination of:
https://lore.kernel.org/lkml/[email protected]/
https://lore.kernel.org/lkml/[email protected]/
Macronix patches will be handled in a further series.
v2:
- Fix bug on octal dtr disable, s/nor->reg_proto/SNOR_PROTO_1_1_1,
because after disable the nor->proto is not yet updated
- update function/macros names to comply with Michael's rename series.
Tudor Ambarus (8):
mtd: spi-nor: Rename method, s/spi_nor_match_id/spi_nor_match_name
mtd: spi-nor: Introduce spi_nor_match_id()
mtd: spi-nor: core: Use auto-detection only once
mtd: spi-nor: core: Introduce method for RDID op
mtd: spi-nor: manufacturers: Use spi_nor_read_id() core method
mtd: spi-nor: core: Add helpers to read/write any register
mtd: spi-nor: micron-st: Rework spi_nor_micron_octal_dtr_enable()
mtd: spi-nor: spansion: Rework spi_nor_cypress_octal_dtr_enable()
drivers/mtd/spi-nor/core.c | 153 ++++++++++++++++++++++----------
drivers/mtd/spi-nor/core.h | 13 +++
drivers/mtd/spi-nor/micron-st.c | 112 ++++++++++++-----------
drivers/mtd/spi-nor/spansion.c | 131 ++++++++++++++-------------
4 files changed, 241 insertions(+), 168 deletions(-)
--
2.25.1
Introduce template operation to remove code duplication.
Split spi_nor_micron_octal_dtr_enable() in spi_nor_micron_octal_dtr_en()
and spi_nor_micron_octal_dtr_dis() as it no longer made sense to try to
keep everything alltogether: too many "if (enable)" throughout the code,
which made the code difficult to follow.
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/mtd/spi-nor/micron-st.c | 105 +++++++++++++++++---------------
1 file changed, 55 insertions(+), 50 deletions(-)
diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
index 41b87868ecf9..12ec3660fd6f 100644
--- a/drivers/mtd/spi-nor/micron-st.c
+++ b/drivers/mtd/spi-nor/micron-st.c
@@ -28,73 +28,72 @@
#define FSR_P_ERR BIT(4) /* Program operation status */
#define FSR_PT_ERR BIT(1) /* Protection error bit */
-static int micron_st_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)
+/* Micron ST SPI NOR flash operations. */
+#define MICRON_ST_NOR_WR_ANY_REG_OP(naddr, addr, ndata, buf) \
+ SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_MT_WR_ANY_REG, 0), \
+ SPI_MEM_OP_ADDR(naddr, addr, 0), \
+ SPI_MEM_OP_NO_DUMMY, \
+ SPI_MEM_OP_DATA_OUT(ndata, buf, 0))
+
+static int micron_st_nor_octal_dtr_en(struct spi_nor *nor)
{
struct spi_mem_op op;
u8 *buf = nor->bouncebuf;
int ret;
- if (enable) {
- /* Use 20 dummy cycles for memory array reads. */
- ret = spi_nor_write_enable(nor);
- if (ret)
- return ret;
-
- *buf = 20;
- op = (struct spi_mem_op)
- SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_MT_WR_ANY_REG, 1),
- SPI_MEM_OP_ADDR(3, SPINOR_REG_MT_CFR1V, 1),
- SPI_MEM_OP_NO_DUMMY,
- SPI_MEM_OP_DATA_OUT(1, buf, 1));
-
- ret = spi_mem_exec_op(nor->spimem, &op);
- if (ret)
- return ret;
+ /* Use 20 dummy cycles for memory array reads. */
+ *buf = 20;
+ op = (struct spi_mem_op)
+ MICRON_ST_NOR_WR_ANY_REG_OP(3, SPINOR_REG_MT_CFR1V, 1, buf);
+ ret = spi_nor_write_reg(nor, &op, nor->reg_proto);
+ if (ret)
+ return ret;
+ ret = spi_nor_wait_till_ready(nor);
+ if (ret)
+ return ret;
- ret = spi_nor_wait_till_ready(nor);
- if (ret)
- return ret;
- }
+ buf[0] = SPINOR_MT_OCT_DTR;
+ op = (struct spi_mem_op)
+ MICRON_ST_NOR_WR_ANY_REG_OP(3, SPINOR_REG_MT_CFR0V, 1, buf);
+ ret = spi_nor_write_reg(nor, &op, nor->reg_proto);
+ if (ret)
+ return ret;
- ret = spi_nor_write_enable(nor);
+ /* Read flash ID to make sure the switch was successful. */
+ ret = spi_nor_read_id(nor, 0, 8, buf, SNOR_PROTO_8_8_8_DTR);
if (ret)
return ret;
- if (enable) {
- buf[0] = SPINOR_MT_OCT_DTR;
- } else {
- /*
- * The register is 1-byte wide, but 1-byte transactions are not
- * allowed in 8D-8D-8D mode. The next register is the dummy
- * cycle configuration register. Since the transaction needs to
- * be at least 2 bytes wide, set the next register to its
- * default value. This also makes sense because the value was
- * changed when enabling 8D-8D-8D mode, it should be reset when
- * disabling.
- */
- buf[0] = SPINOR_MT_EXSPI;
- buf[1] = SPINOR_REG_MT_CFR1V_DEF;
- }
+ if (memcmp(buf, nor->info->id, nor->info->id_len))
+ return -EINVAL;
- op = (struct spi_mem_op)
- SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_MT_WR_ANY_REG, 1),
- SPI_MEM_OP_ADDR(enable ? 3 : 4,
- SPINOR_REG_MT_CFR0V, 1),
- SPI_MEM_OP_NO_DUMMY,
- SPI_MEM_OP_DATA_OUT(enable ? 1 : 2, buf, 1));
+ return 0;
+}
- if (!enable)
- spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
+static int micron_st_nor_octal_dtr_dis(struct spi_nor *nor)
+{
+ struct spi_mem_op op;
+ u8 *buf = nor->bouncebuf;
+ int ret;
- ret = spi_mem_exec_op(nor->spimem, &op);
+ /*
+ * The register is 1-byte wide, but 1-byte transactions are not allowed
+ * in 8D-8D-8D mode. The next register is the dummy cycle configuration
+ * register. Since the transaction needs to be at least 2 bytes wide,
+ * set the next register to its default value. This also makes sense
+ * because the value was changed when enabling 8D-8D-8D mode, it should
+ * be reset when disabling.
+ */
+ buf[0] = SPINOR_MT_EXSPI;
+ buf[1] = SPINOR_REG_MT_CFR1V_DEF;
+ op = (struct spi_mem_op)
+ MICRON_ST_NOR_WR_ANY_REG_OP(4, SPINOR_REG_MT_CFR0V, 2, buf);
+ ret = spi_nor_write_reg(nor, &op, SNOR_PROTO_8_8_8_DTR);
if (ret)
return ret;
/* Read flash ID to make sure the switch was successful. */
- if (enable)
- ret = spi_nor_read_id(nor, 0, 8, buf, SNOR_PROTO_8_8_8_DTR);
- else
- ret = spi_nor_read_id(nor, 0, 0, buf, SNOR_PROTO_1_1_1);
+ ret = spi_nor_read_id(nor, 0, 0, buf, SNOR_PROTO_1_1_1);
if (ret)
return ret;
@@ -104,6 +103,12 @@ static int micron_st_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)
return 0;
}
+static int micron_st_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)
+{
+ return enable ? micron_st_nor_octal_dtr_en(nor) :
+ micron_st_nor_octal_dtr_dis(nor);
+}
+
static void mt35xu512aba_default_init(struct spi_nor *nor)
{
nor->params->octal_dtr_enable = micron_st_nor_octal_dtr_enable;
--
2.25.1
RDID is used in the core to auto detect the flash, but also by some
manufacturer drivers that contain flashes that support Octal DTR mode,
so that they can read the flash ID after the switch to Octal DTR was made
to test if the switch was successful. Introduce a core method for RDID op
to avoid code duplication.
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/mtd/spi-nor/core.c | 58 ++++++++++++++++++++++++++------------
drivers/mtd/spi-nor/core.h | 9 ++++++
2 files changed, 49 insertions(+), 18 deletions(-)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index b1d6fa65417d..281e3d25f74c 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -369,6 +369,41 @@ int spi_nor_write_disable(struct spi_nor *nor)
return ret;
}
+/**
+ * spi_nor_read_id() - Read the JEDEC ID.
+ * @nor: pointer to 'struct spi_nor'.
+ * @naddr: number of address bytes to send. Can be zero if the operation
+ * does not need to send an address.
+ * @ndummy: number of dummy bytes to send after an opcode or address. Can
+ * be zero if the operation does not require dummy bytes.
+ * @id: pointer to a DMA-able buffer where the value of the JEDEC ID
+ * will be written.
+ * @reg_proto: the SPI protocol for register operation.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id,
+ enum spi_nor_protocol reg_proto)
+{
+ int ret;
+
+ if (nor->spimem) {
+ struct spi_mem_op op =
+ SPI_NOR_READID_OP(naddr, ndummy, id, SPI_NOR_MAX_ID_LEN);
+
+ spi_nor_spimem_setup_op(nor, &op, reg_proto);
+ ret = spi_mem_exec_op(nor->spimem, &op);
+ } else {
+ ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id,
+ SPI_NOR_MAX_ID_LEN);
+ }
+
+ if (ret)
+ dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret);
+
+ return ret;
+}
+
/**
* spi_nor_read_sr() - Read the Status Register.
* @nor: pointer to 'struct spi_nor'.
@@ -1649,28 +1684,15 @@ static const struct flash_info *spi_nor_match_id(struct spi_nor *nor,
return NULL;
}
-static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
+static const struct flash_info *spi_nor_detect(struct spi_nor *nor)
{
const struct flash_info *info;
u8 *id = nor->bouncebuf;
int ret;
- if (nor->spimem) {
- struct spi_mem_op op =
- SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1),
- SPI_MEM_OP_NO_ADDR,
- SPI_MEM_OP_NO_DUMMY,
- SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, id, 1));
-
- ret = spi_mem_exec_op(nor->spimem, &op);
- } else {
- ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id,
- SPI_NOR_MAX_ID_LEN);
- }
- if (ret) {
- dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret);
+ ret = spi_nor_read_id(nor, 0, 0, id, nor->reg_proto);
+ if (ret)
return ERR_PTR(ret);
- }
info = spi_nor_match_id(nor, id);
if (!info) {
@@ -2900,7 +2922,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
info = spi_nor_match_name(nor, name);
/* Try to auto-detect if chip name wasn't specified or not found */
if (!info) {
- detected_info = spi_nor_read_id(nor);
+ detected_info = spi_nor_detect(nor);
info = detected_info;
}
if (IS_ERR_OR_NULL(info))
@@ -2913,7 +2935,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
if (name && !detected_info && info->id_len) {
const struct flash_info *jinfo;
- jinfo = spi_nor_read_id(nor);
+ jinfo = spi_nor_detect(nor);
if (IS_ERR(jinfo)) {
return jinfo;
} else if (jinfo != info) {
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index b7fd760e3b47..f952061d5c24 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -11,6 +11,13 @@
#define SPI_NOR_MAX_ID_LEN 6
+/* Standard SPI NOR flash operations. */
+#define SPI_NOR_READID_OP(naddr, ndummy, buf, len) \
+ SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 0), \
+ SPI_MEM_OP_ADDR(naddr, 0, 0), \
+ SPI_MEM_OP_DUMMY(ndummy, 0), \
+ SPI_MEM_OP_DATA_IN(len, buf, 0))
+
enum spi_nor_option_flags {
SNOR_F_HAS_SR_TB = BIT(0),
SNOR_F_NO_OP_CHIP_ERASE = BIT(1),
@@ -534,6 +541,8 @@ void spi_nor_unlock_and_unprep(struct spi_nor *nor);
int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor);
int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor);
int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor);
+int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id,
+ enum spi_nor_protocol reg_proto);
int spi_nor_read_sr(struct spi_nor *nor, u8 *sr);
int spi_nor_sr_ready(struct spi_nor *nor);
int spi_nor_read_cr(struct spi_nor *nor, u8 *cr);
--
2.25.1
There are manufacturers that use registers indexed by address. Some of
them support "read/write any register" opcodes. Provide core methods that
can be used by all manufacturers. SPI NOR controller ops are intentionally
not supported as we intend to move all the SPI NOR controller drivers
under the SPI subsystem.
Signed-off-by: Tudor Ambarus <[email protected]>
Tested-by: Takahiro Kuwano <[email protected]>
Reviewed-by: Pratyush Yadav <[email protected]>
---
drivers/mtd/spi-nor/core.c | 41 ++++++++++++++++++++++++++++++++++++++
drivers/mtd/spi-nor/core.h | 4 ++++
2 files changed, 45 insertions(+)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 281e3d25f74c..f1aa1e2ea7c9 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -307,6 +307,47 @@ ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
return nor->controller_ops->write(nor, to, len, buf);
}
+/**
+ * spi_nor_read_reg() - read register to flash memory
+ * @nor: pointer to 'struct spi_nor'.
+ * @op: SPI memory operation. op->data.buf must be DMA-able.
+ * @proto: SPI protocol to use for the register operation.
+ *
+ * Return: zero on success, -errno otherwise
+ */
+int spi_nor_read_reg(struct spi_nor *nor, struct spi_mem_op *op,
+ enum spi_nor_protocol proto)
+{
+ if (!nor->spimem)
+ return -EOPNOTSUPP;
+
+ spi_nor_spimem_setup_op(nor, op, proto);
+ return spi_nor_spimem_exec_op(nor, op);
+}
+
+/**
+ * spi_nor_write_reg() - write register to flash memory
+ * @nor: pointer to 'struct spi_nor'
+ * @op: SPI memory operation. op->data.buf must be DMA-able.
+ * @proto: SPI protocol to use for the register operation.
+ *
+ * Return: zero on success, -errno otherwise
+ */
+int spi_nor_write_reg(struct spi_nor *nor, struct spi_mem_op *op,
+ enum spi_nor_protocol proto)
+{
+ int ret;
+
+ if (!nor->spimem)
+ return -EOPNOTSUPP;
+
+ ret = spi_nor_write_enable(nor);
+ if (ret)
+ return ret;
+ spi_nor_spimem_setup_op(nor, op, proto);
+ return spi_nor_spimem_exec_op(nor, op);
+}
+
/**
* spi_nor_write_enable() - Set write enable latch with Write Enable command.
* @nor: pointer to 'struct spi_nor'.
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index f952061d5c24..7c704475946d 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -554,6 +554,10 @@ ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
u8 *buf);
ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
const u8 *buf);
+int spi_nor_read_reg(struct spi_nor *nor, struct spi_mem_op *op,
+ enum spi_nor_protocol proto);
+int spi_nor_write_reg(struct spi_nor *nor, struct spi_mem_op *op,
+ enum spi_nor_protocol proto);
int spi_nor_erase_sector(struct spi_nor *nor, u32 addr);
int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);
--
2.25.1
In case spi_nor_match_name() returned NULL, the auto detection was
issued twice. There's no reason to try to detect the same chip twice,
do the auto detection only once.
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/mtd/spi-nor/core.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index f87cb7d3daab..b1d6fa65417d 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2894,13 +2894,15 @@ static const struct flash_info *spi_nor_match_name(struct spi_nor *nor,
static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
const char *name)
{
- const struct flash_info *info = NULL;
+ const struct flash_info *info = NULL, *detected_info = NULL;
if (name)
info = spi_nor_match_name(nor, name);
/* Try to auto-detect if chip name wasn't specified or not found */
- if (!info)
- info = spi_nor_read_id(nor);
+ if (!info) {
+ detected_info = spi_nor_read_id(nor);
+ info = detected_info;
+ }
if (IS_ERR_OR_NULL(info))
return ERR_PTR(-ENOENT);
@@ -2908,7 +2910,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
* If caller has specified name of flash model that can normally be
* detected using JEDEC, let's verify it.
*/
- if (name && info->id_len) {
+ if (name && !detected_info && info->id_len) {
const struct flash_info *jinfo;
jinfo = spi_nor_read_id(nor);
--
2.25.1
Similar to spi_nor_match_name() extend the search of flash_info through
all the manufacturers, this time doing the match by ID. There's no reason
to limit the search per manufacturer yet, do it globally, search the flash
in all the parts of all manufacturers in a single method.
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/mtd/spi-nor/core.c | 40 ++++++++++++++++++--------------------
1 file changed, 19 insertions(+), 21 deletions(-)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index f3c359d03163..f87cb7d3daab 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1629,16 +1629,21 @@ static const struct spi_nor_manufacturer *manufacturers[] = {
&spi_nor_xmc,
};
-static const struct flash_info *
-spi_nor_search_part_by_id(const struct flash_info *parts, unsigned int nparts,
- const u8 *id)
+static const struct flash_info *spi_nor_match_id(struct spi_nor *nor,
+ const u8 *id)
{
- unsigned int i;
+ const struct flash_info *part;
+ unsigned int i, j;
- for (i = 0; i < nparts; i++) {
- if (parts[i].id_len &&
- !memcmp(parts[i].id, id, parts[i].id_len))
- return &parts[i];
+ for (i = 0; i < ARRAY_SIZE(manufacturers); i++) {
+ for (j = 0; j < manufacturers[i]->nparts; j++) {
+ part = &manufacturers[i]->parts[j];
+ if (part->id_len &&
+ !memcmp(part->id, id, part->id_len)) {
+ nor->manufacturer = manufacturers[i];
+ return part;
+ }
+ }
}
return NULL;
@@ -1648,7 +1653,6 @@ static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
{
const struct flash_info *info;
u8 *id = nor->bouncebuf;
- unsigned int i;
int ret;
if (nor->spimem) {
@@ -1668,19 +1672,13 @@ static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
return ERR_PTR(ret);
}
- for (i = 0; i < ARRAY_SIZE(manufacturers); i++) {
- info = spi_nor_search_part_by_id(manufacturers[i]->parts,
- manufacturers[i]->nparts,
- id);
- if (info) {
- nor->manufacturer = manufacturers[i];
- return info;
- }
+ info = spi_nor_match_id(nor, id);
+ if (!info) {
+ dev_err(nor->dev, "unrecognized JEDEC id bytes: %*ph\n",
+ SPI_NOR_MAX_ID_LEN, id);
+ return ERR_PTR(-ENODEV);
}
-
- dev_err(nor->dev, "unrecognized JEDEC id bytes: %*ph\n",
- SPI_NOR_MAX_ID_LEN, id);
- return ERR_PTR(-ENODEV);
+ return info;
}
static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
--
2.25.1
Introduce template operation to remove code duplication.
Split spi_nor_cypress_octal_dtr_enable() in
spi_nor_cypress_octal_dtr_ena() spi_nor_cypress_octal_dtr_dis() as it no
longer made sense to try to keep everything alltogether: too many
"if (enable)" throughout the code, which made the code difficult to read.
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/mtd/spi-nor/spansion.c | 124 +++++++++++++++++----------------
1 file changed, 65 insertions(+), 59 deletions(-)
diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index c5988312cc91..34ca3538a0f9 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -23,85 +23,75 @@
#define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS 0
#define SPINOR_OP_CYPRESS_RD_FAST 0xee
-/**
- * cypress_nor_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
- * @nor: pointer to a 'struct spi_nor'
- * @enable: whether to enable or disable Octal DTR
- *
- * This also sets the memory access latency cycles to 24 to allow the flash to
- * run at up to 200MHz.
- *
- * Return: 0 on success, -errno otherwise.
- */
-static int cypress_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)
+/* Cypress SPI NOR flash operations. */
+#define CYPRESS_NOR_WR_ANY_REG_OP(naddr, addr, ndata, buf) \
+ SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_ANY_REG, 0), \
+ SPI_MEM_OP_ADDR(naddr, addr, 0), \
+ SPI_MEM_OP_NO_DUMMY, \
+ SPI_MEM_OP_DATA_OUT(ndata, buf, 0))
+
+static int cypress_nor_octal_dtr_en(struct spi_nor *nor)
{
struct spi_mem_op op;
u8 *buf = nor->bouncebuf;
int ret;
- if (enable) {
- /* Use 24 dummy cycles for memory array reads. */
- ret = spi_nor_write_enable(nor);
- if (ret)
- return ret;
+ /* Use 24 dummy cycles for memory array reads. */
+ *buf = SPINOR_REG_CYPRESS_CFR2V_MEMLAT_11_24;
+ op = (struct spi_mem_op)
+ CYPRESS_NOR_WR_ANY_REG_OP(3, SPINOR_REG_CYPRESS_CFR2V, 1, buf);
- *buf = SPINOR_REG_CYPRESS_CFR2V_MEMLAT_11_24;
- op = (struct spi_mem_op)
- SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_ANY_REG, 1),
- SPI_MEM_OP_ADDR(3, SPINOR_REG_CYPRESS_CFR2V,
- 1),
- SPI_MEM_OP_NO_DUMMY,
- SPI_MEM_OP_DATA_OUT(1, buf, 1));
+ ret = spi_nor_write_reg(nor, &op, nor->reg_proto);
+ if (ret)
+ return ret;
- ret = spi_mem_exec_op(nor->spimem, &op);
- if (ret)
- return ret;
+ ret = spi_nor_wait_till_ready(nor);
+ if (ret)
+ return ret;
- ret = spi_nor_wait_till_ready(nor);
- if (ret)
- return ret;
+ nor->read_dummy = 24;
- nor->read_dummy = 24;
- }
+ /* Set the octal and DTR enable bits. */
+ buf[0] = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN;
+ op = (struct spi_mem_op)
+ CYPRESS_NOR_WR_ANY_REG_OP(3, SPINOR_REG_CYPRESS_CFR5V, 1, buf);
- /* Set/unset the octal and DTR enable bits. */
- ret = spi_nor_write_enable(nor);
+ ret = spi_nor_write_reg(nor, &op, nor->reg_proto);
if (ret)
return ret;
- if (enable) {
- buf[0] = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN;
- } else {
- /*
- * The register is 1-byte wide, but 1-byte transactions are not
- * allowed in 8D-8D-8D mode. Since there is no register at the
- * next location, just initialize the value to 0 and let the
- * transaction go on.
- */
- buf[0] = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS;
- buf[1] = 0;
- }
+ /* Read flash ID to make sure the switch was successful. */
+ ret = spi_nor_read_id(nor, 4, 3, buf, SNOR_PROTO_8_8_8_DTR);
+ if (ret)
+ return ret;
- op = (struct spi_mem_op)
- SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_ANY_REG, 1),
- SPI_MEM_OP_ADDR(enable ? 3 : 4,
- SPINOR_REG_CYPRESS_CFR5V,
- 1),
- SPI_MEM_OP_NO_DUMMY,
- SPI_MEM_OP_DATA_OUT(enable ? 1 : 2, buf, 1));
+ if (memcmp(buf, nor->info->id, nor->info->id_len))
+ return -EINVAL;
- if (!enable)
- spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
+ return 0;
+}
- ret = spi_mem_exec_op(nor->spimem, &op);
+static int cypress_nor_octal_dtr_dis(struct spi_nor *nor)
+{
+ struct spi_mem_op op;
+ u8 *buf = nor->bouncebuf;
+ int ret;
+
+ /*
+ * The register is 1-byte wide, but 1-byte transactions are not allowed
+ * in 8D-8D-8D mode. Since there is no register at the next location,
+ * just initialize the value to 0 and let the transaction go on.
+ */
+ buf[0] = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS;
+ buf[1] = 0;
+ op = (struct spi_mem_op)
+ CYPRESS_NOR_WR_ANY_REG_OP(4, SPINOR_REG_CYPRESS_CFR5V, 2, buf);
+ ret = spi_nor_write_reg(nor, &op, SNOR_PROTO_8_8_8_DTR);
if (ret)
return ret;
/* Read flash ID to make sure the switch was successful. */
- if (enable)
- ret = spi_nor_read_id(nor, 4, 3, buf, SNOR_PROTO_8_8_8_DTR);
- else
- ret = spi_nor_read_id(nor, 0, 0, buf, SNOR_PROTO_1_1_1);
+ ret = spi_nor_read_id(nor, 0, 0, buf, SNOR_PROTO_1_1_1);
if (ret)
return ret;
@@ -111,6 +101,22 @@ static int cypress_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)
return 0;
}
+/**
+ * cypress_nor_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
+ * @nor: pointer to a 'struct spi_nor'
+ * @enable: whether to enable or disable Octal DTR
+ *
+ * This also sets the memory access latency cycles to 24 to allow the flash to
+ * run at up to 200MHz.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int cypress_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)
+{
+ return enable ? cypress_nor_octal_dtr_en(nor) :
+ cypress_nor_octal_dtr_dis(nor);
+}
+
static void s28hs512t_default_init(struct spi_nor *nor)
{
nor->params->octal_dtr_enable = cypress_nor_octal_dtr_enable;
--
2.25.1
On 28/02/22 01:17PM, Tudor Ambarus wrote:
> In case spi_nor_match_name() returned NULL, the auto detection was
> issued twice. There's no reason to try to detect the same chip twice,
> do the auto detection only once.
>
> Signed-off-by: Tudor Ambarus <[email protected]>
> ---
> drivers/mtd/spi-nor/core.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index f87cb7d3daab..b1d6fa65417d 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2894,13 +2894,15 @@ static const struct flash_info *spi_nor_match_name(struct spi_nor *nor,
> static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
> const char *name)
> {
> - const struct flash_info *info = NULL;
> + const struct flash_info *info = NULL, *detected_info = NULL;
>
> if (name)
> info = spi_nor_match_name(nor, name);
> /* Try to auto-detect if chip name wasn't specified or not found */
> - if (!info)
> - info = spi_nor_read_id(nor);
> + if (!info) {
> + detected_info = spi_nor_read_id(nor);
> + info = detected_info;
> + }
> if (IS_ERR_OR_NULL(info))
> return ERR_PTR(-ENOENT);
>
> @@ -2908,7 +2910,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
> * If caller has specified name of flash model that can normally be
> * detected using JEDEC, let's verify it.
> */
> - if (name && info->id_len) {
> + if (name && !detected_info && info->id_len) {
> const struct flash_info *jinfo;
>
> jinfo = spi_nor_read_id(nor);
I think the flow can be a little bit better. How about:
if (name)
info = spi_nor_match_name();
if (!info) {
info = spi_nor_read_id();
if (IS_ERR_OR_NULL(info))
return ERR_PTR(-ENOENT);
return info;
}
if (name && info->id_len) {
...
}
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
On 21/03/22 12:50PM, [email protected] wrote:
> On 3/21/22 14:14, Pratyush Yadav wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > On 28/02/22 01:17PM, Tudor Ambarus wrote:
> >> In case spi_nor_match_name() returned NULL, the auto detection was
> >> issued twice. There's no reason to try to detect the same chip twice,
> >> do the auto detection only once.
> >>
> >> Signed-off-by: Tudor Ambarus <[email protected]>
> >> ---
> >> drivers/mtd/spi-nor/core.c | 10 ++++++----
> >> 1 file changed, 6 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> >> index f87cb7d3daab..b1d6fa65417d 100644
> >> --- a/drivers/mtd/spi-nor/core.c
> >> +++ b/drivers/mtd/spi-nor/core.c
> >> @@ -2894,13 +2894,15 @@ static const struct flash_info *spi_nor_match_name(struct spi_nor *nor,
> >> static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
> >> const char *name)
> >> {
> >> - const struct flash_info *info = NULL;
> >> + const struct flash_info *info = NULL, *detected_info = NULL;
> >>
> >> if (name)
> >> info = spi_nor_match_name(nor, name);
> >> /* Try to auto-detect if chip name wasn't specified or not found */
> >> - if (!info)
> >> - info = spi_nor_read_id(nor);
> >> + if (!info) {
> >> + detected_info = spi_nor_read_id(nor);
> >> + info = detected_info;
> >> + }
> >> if (IS_ERR_OR_NULL(info))
> >> return ERR_PTR(-ENOENT);
> >>
> >> @@ -2908,7 +2910,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
> >> * If caller has specified name of flash model that can normally be
> >> * detected using JEDEC, let's verify it.
> >> */
> >> - if (name && info->id_len) {
> >> + if (name && !detected_info && info->id_len) {
> >> const struct flash_info *jinfo;
> >>
> >> jinfo = spi_nor_read_id(nor);
> >
> > I think the flow can be a little bit better. How about:
> >
> > if (name)
> > info = spi_nor_match_name();
> >
> > if (!info) {
> > info = spi_nor_read_id();
> > if (IS_ERR_OR_NULL(info))
> > return ERR_PTR(-ENOENT);
> >
> > return info;
> > }
>
> Here we miss the IS_ERR check in case info is retrieved with spi_nor_match_name().
> Do you expect spi_nor_match_name() to ever return an error? As it is now it doesn't.
> I'm fine either way. In case you want me to follow your suggestion, give me a sign
> and I'll make a dedicated patch to move the IS_ERR_OR_NULL check. Will add your
> Suggested-by tag.
I think it should be safe to assume it won't ever return an error since
all it does is iterate over an array that is always present. I don't see
that changing in the foreseeable future either. So I think not having
the IS_ERR check is fine.
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
On 3/21/22 14:34, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 28/02/22 01:17PM, Tudor Ambarus wrote:
>> Introduce template operation to remove code duplication.
>> Split spi_nor_cypress_octal_dtr_enable() in
>> spi_nor_cypress_octal_dtr_ena() spi_nor_cypress_octal_dtr_dis() as it no
>> longer made sense to try to keep everything alltogether: too many
>> "if (enable)" throughout the code, which made the code difficult to read.
>>
>> Signed-off-by: Tudor Ambarus <[email protected]>
>
> Reviewed-by: Pratyush Yadav <[email protected]>
>
> Will send my tested-by separately once I get a chance to test this.
>
Thank you!
On 28/02/22 01:17PM, Tudor Ambarus wrote:
> Introduce template operation to remove code duplication.
> Split spi_nor_micron_octal_dtr_enable() in spi_nor_micron_octal_dtr_en()
> and spi_nor_micron_octal_dtr_dis() as it no longer made sense to try to
> keep everything alltogether: too many "if (enable)" throughout the code,
> which made the code difficult to follow.
>
> Signed-off-by: Tudor Ambarus <[email protected]>
Reviewed-by: Pratyush Yadav <[email protected]>
Will send my tested-by separately once I get a chance to test this.
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
On 3/21/22 14:21, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 28/02/22 01:17PM, Tudor Ambarus wrote:
>> RDID is used in the core to auto detect the flash, but also by some
>> manufacturer drivers that contain flashes that support Octal DTR mode,
>> so that they can read the flash ID after the switch to Octal DTR was made
>> to test if the switch was successful. Introduce a core method for RDID op
>> to avoid code duplication.
>>
>> Signed-off-by: Tudor Ambarus <[email protected]>
>> ---
>> drivers/mtd/spi-nor/core.c | 58 ++++++++++++++++++++++++++------------
>> drivers/mtd/spi-nor/core.h | 9 ++++++
>> 2 files changed, 49 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index b1d6fa65417d..281e3d25f74c 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -369,6 +369,41 @@ int spi_nor_write_disable(struct spi_nor *nor)
>> return ret;
>> }
>>
>> +/**
>> + * spi_nor_read_id() - Read the JEDEC ID.
>> + * @nor: pointer to 'struct spi_nor'.
>> + * @naddr: number of address bytes to send. Can be zero if the operation
>> + * does not need to send an address.
>> + * @ndummy: number of dummy bytes to send after an opcode or address. Can
>> + * be zero if the operation does not require dummy bytes.
>> + * @id: pointer to a DMA-able buffer where the value of the JEDEC ID
>> + * will be written.
>> + * @reg_proto: the SPI protocol for register operation.
>> + *
>> + * Return: 0 on success, -errno otherwise.
>> + */
>> +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id,
>> + enum spi_nor_protocol reg_proto)
>
> Nitpick: Could just call it 'proto'.
sure, will update
>
>> +{
>> + int ret;
>> +
>> + if (nor->spimem) {
>> + struct spi_mem_op op =
>> + SPI_NOR_READID_OP(naddr, ndummy, id, SPI_NOR_MAX_ID_LEN);
>> +
>> + spi_nor_spimem_setup_op(nor, &op, reg_proto);
>> + ret = spi_mem_exec_op(nor->spimem, &op);
>> + } else {
>> + ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id,
>> + SPI_NOR_MAX_ID_LEN);
>> + }
>> +
>> + if (ret)
>> + dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret);
>
> I think this message should be in spi_nor_detect(). Let octal DTR enable
As of now every SPI NOR operation that return an error also prints a dbg
message. I like this because it offers a smaller granularity on the error
cause.
> methods print their own, more specific error messages.
How about duplicating the error in the octal dtr enable methods if you
feel it is worth it?
>
>> +
>> + return ret;
>> +}
>> +
>> /**
>> * spi_nor_read_sr() - Read the Status Register.
>> * @nor: pointer to 'struct spi_nor'.
>> @@ -1649,28 +1684,15 @@ static const struct flash_info *spi_nor_match_id(struct spi_nor *nor,
>> return NULL;
>> }
>>
>> -static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
>> +static const struct flash_info *spi_nor_detect(struct spi_nor *nor)
>> {
>> const struct flash_info *info;
>> u8 *id = nor->bouncebuf;
>> int ret;
>>
>> - if (nor->spimem) {
>> - struct spi_mem_op op =
>> - SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1),
>> - SPI_MEM_OP_NO_ADDR,
>> - SPI_MEM_OP_NO_DUMMY,
>> - SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, id, 1));
>> -
>> - ret = spi_mem_exec_op(nor->spimem, &op);
>> - } else {
>> - ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id,
>> - SPI_NOR_MAX_ID_LEN);
>> - }
>> - if (ret) {
>> - dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret);
>> + ret = spi_nor_read_id(nor, 0, 0, id, nor->reg_proto);
>
> Hmm, I wonder if it is better to explicitly use SNOR_PROTO_1_1_1 so
> clearly signify that this is intended to use 1S-1S-1S only. What do you
> think?
I would keep it as it is for now, because it offers flexibility.
If we ever gonna determine the protocol at runtime this will come in handy
because it will work without touching the code. JESD216 suggests an algorithm
that tries to determine the mode depending on the SFDP signature.
Cheers,
ta
On 28/02/22 01:17PM, Tudor Ambarus wrote:
> Introduce template operation to remove code duplication.
> Split spi_nor_cypress_octal_dtr_enable() in
> spi_nor_cypress_octal_dtr_ena() spi_nor_cypress_octal_dtr_dis() as it no
> longer made sense to try to keep everything alltogether: too many
> "if (enable)" throughout the code, which made the code difficult to read.
>
> Signed-off-by: Tudor Ambarus <[email protected]>
Reviewed-by: Pratyush Yadav <[email protected]>
Will send my tested-by separately once I get a chance to test this.
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
On 28/02/22 01:17PM, Tudor Ambarus wrote:
> RDID is used in the core to auto detect the flash, but also by some
> manufacturer drivers that contain flashes that support Octal DTR mode,
> so that they can read the flash ID after the switch to Octal DTR was made
> to test if the switch was successful. Introduce a core method for RDID op
> to avoid code duplication.
>
> Signed-off-by: Tudor Ambarus <[email protected]>
> ---
> drivers/mtd/spi-nor/core.c | 58 ++++++++++++++++++++++++++------------
> drivers/mtd/spi-nor/core.h | 9 ++++++
> 2 files changed, 49 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index b1d6fa65417d..281e3d25f74c 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -369,6 +369,41 @@ int spi_nor_write_disable(struct spi_nor *nor)
> return ret;
> }
>
> +/**
> + * spi_nor_read_id() - Read the JEDEC ID.
> + * @nor: pointer to 'struct spi_nor'.
> + * @naddr: number of address bytes to send. Can be zero if the operation
> + * does not need to send an address.
> + * @ndummy: number of dummy bytes to send after an opcode or address. Can
> + * be zero if the operation does not require dummy bytes.
> + * @id: pointer to a DMA-able buffer where the value of the JEDEC ID
> + * will be written.
> + * @reg_proto: the SPI protocol for register operation.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id,
> + enum spi_nor_protocol reg_proto)
Nitpick: Could just call it 'proto'.
> +{
> + int ret;
> +
> + if (nor->spimem) {
> + struct spi_mem_op op =
> + SPI_NOR_READID_OP(naddr, ndummy, id, SPI_NOR_MAX_ID_LEN);
> +
> + spi_nor_spimem_setup_op(nor, &op, reg_proto);
> + ret = spi_mem_exec_op(nor->spimem, &op);
> + } else {
> + ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id,
> + SPI_NOR_MAX_ID_LEN);
> + }
> +
> + if (ret)
> + dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret);
I think this message should be in spi_nor_detect(). Let octal DTR enable
methods print their own, more specific error messages.
> +
> + return ret;
> +}
> +
> /**
> * spi_nor_read_sr() - Read the Status Register.
> * @nor: pointer to 'struct spi_nor'.
> @@ -1649,28 +1684,15 @@ static const struct flash_info *spi_nor_match_id(struct spi_nor *nor,
> return NULL;
> }
>
> -static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
> +static const struct flash_info *spi_nor_detect(struct spi_nor *nor)
> {
> const struct flash_info *info;
> u8 *id = nor->bouncebuf;
> int ret;
>
> - if (nor->spimem) {
> - struct spi_mem_op op =
> - SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1),
> - SPI_MEM_OP_NO_ADDR,
> - SPI_MEM_OP_NO_DUMMY,
> - SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, id, 1));
> -
> - ret = spi_mem_exec_op(nor->spimem, &op);
> - } else {
> - ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id,
> - SPI_NOR_MAX_ID_LEN);
> - }
> - if (ret) {
> - dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret);
> + ret = spi_nor_read_id(nor, 0, 0, id, nor->reg_proto);
Hmm, I wonder if it is better to explicitly use SNOR_PROTO_1_1_1 so
clearly signify that this is intended to use 1S-1S-1S only. What do you
think?
> + if (ret)
> return ERR_PTR(ret);
> - }
>
> info = spi_nor_match_id(nor, id);
> if (!info) {
> @@ -2900,7 +2922,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
> info = spi_nor_match_name(nor, name);
> /* Try to auto-detect if chip name wasn't specified or not found */
> if (!info) {
> - detected_info = spi_nor_read_id(nor);
> + detected_info = spi_nor_detect(nor);
> info = detected_info;
> }
> if (IS_ERR_OR_NULL(info))
> @@ -2913,7 +2935,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
> if (name && !detected_info && info->id_len) {
> const struct flash_info *jinfo;
>
> - jinfo = spi_nor_read_id(nor);
> + jinfo = spi_nor_detect(nor);
> if (IS_ERR(jinfo)) {
> return jinfo;
> } else if (jinfo != info) {
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index b7fd760e3b47..f952061d5c24 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -11,6 +11,13 @@
>
> #define SPI_NOR_MAX_ID_LEN 6
>
> +/* Standard SPI NOR flash operations. */
> +#define SPI_NOR_READID_OP(naddr, ndummy, buf, len) \
> + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 0), \
> + SPI_MEM_OP_ADDR(naddr, 0, 0), \
> + SPI_MEM_OP_DUMMY(ndummy, 0), \
> + SPI_MEM_OP_DATA_IN(len, buf, 0))
> +
> enum spi_nor_option_flags {
> SNOR_F_HAS_SR_TB = BIT(0),
> SNOR_F_NO_OP_CHIP_ERASE = BIT(1),
> @@ -534,6 +541,8 @@ void spi_nor_unlock_and_unprep(struct spi_nor *nor);
> int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor);
> int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor);
> int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor);
> +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id,
> + enum spi_nor_protocol reg_proto);
> int spi_nor_read_sr(struct spi_nor *nor, u8 *sr);
> int spi_nor_sr_ready(struct spi_nor *nor);
> int spi_nor_read_cr(struct spi_nor *nor, u8 *cr);
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
On 3/21/22 14:14, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 28/02/22 01:17PM, Tudor Ambarus wrote:
>> In case spi_nor_match_name() returned NULL, the auto detection was
>> issued twice. There's no reason to try to detect the same chip twice,
>> do the auto detection only once.
>>
>> Signed-off-by: Tudor Ambarus <[email protected]>
>> ---
>> drivers/mtd/spi-nor/core.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index f87cb7d3daab..b1d6fa65417d 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -2894,13 +2894,15 @@ static const struct flash_info *spi_nor_match_name(struct spi_nor *nor,
>> static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
>> const char *name)
>> {
>> - const struct flash_info *info = NULL;
>> + const struct flash_info *info = NULL, *detected_info = NULL;
>>
>> if (name)
>> info = spi_nor_match_name(nor, name);
>> /* Try to auto-detect if chip name wasn't specified or not found */
>> - if (!info)
>> - info = spi_nor_read_id(nor);
>> + if (!info) {
>> + detected_info = spi_nor_read_id(nor);
>> + info = detected_info;
>> + }
>> if (IS_ERR_OR_NULL(info))
>> return ERR_PTR(-ENOENT);
>>
>> @@ -2908,7 +2910,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
>> * If caller has specified name of flash model that can normally be
>> * detected using JEDEC, let's verify it.
>> */
>> - if (name && info->id_len) {
>> + if (name && !detected_info && info->id_len) {
>> const struct flash_info *jinfo;
>>
>> jinfo = spi_nor_read_id(nor);
>
> I think the flow can be a little bit better. How about:
>
> if (name)
> info = spi_nor_match_name();
>
> if (!info) {
> info = spi_nor_read_id();
> if (IS_ERR_OR_NULL(info))
> return ERR_PTR(-ENOENT);
>
> return info;
> }
Here we miss the IS_ERR check in case info is retrieved with spi_nor_match_name().
Do you expect spi_nor_match_name() to ever return an error? As it is now it doesn't.
I'm fine either way. In case you want me to follow your suggestion, give me a sign
and I'll make a dedicated patch to move the IS_ERR_OR_NULL check. Will add your
Suggested-by tag.
Cheers,
ta
On 21/03/22 01:18PM, [email protected] wrote:
> On 3/21/22 14:21, Pratyush Yadav wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > On 28/02/22 01:17PM, Tudor Ambarus wrote:
> >> RDID is used in the core to auto detect the flash, but also by some
> >> manufacturer drivers that contain flashes that support Octal DTR mode,
> >> so that they can read the flash ID after the switch to Octal DTR was made
> >> to test if the switch was successful. Introduce a core method for RDID op
> >> to avoid code duplication.
> >>
> >> Signed-off-by: Tudor Ambarus <[email protected]>
> >> ---
> >> drivers/mtd/spi-nor/core.c | 58 ++++++++++++++++++++++++++------------
> >> drivers/mtd/spi-nor/core.h | 9 ++++++
> >> 2 files changed, 49 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> >> index b1d6fa65417d..281e3d25f74c 100644
> >> --- a/drivers/mtd/spi-nor/core.c
> >> +++ b/drivers/mtd/spi-nor/core.c
> >> @@ -369,6 +369,41 @@ int spi_nor_write_disable(struct spi_nor *nor)
> >> return ret;
> >> }
> >>
> >> +/**
> >> + * spi_nor_read_id() - Read the JEDEC ID.
> >> + * @nor: pointer to 'struct spi_nor'.
> >> + * @naddr: number of address bytes to send. Can be zero if the operation
> >> + * does not need to send an address.
> >> + * @ndummy: number of dummy bytes to send after an opcode or address. Can
> >> + * be zero if the operation does not require dummy bytes.
> >> + * @id: pointer to a DMA-able buffer where the value of the JEDEC ID
> >> + * will be written.
> >> + * @reg_proto: the SPI protocol for register operation.
> >> + *
> >> + * Return: 0 on success, -errno otherwise.
> >> + */
> >> +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id,
> >> + enum spi_nor_protocol reg_proto)
> >
> > Nitpick: Could just call it 'proto'.
>
> sure, will update
>
> >
> >> +{
> >> + int ret;
> >> +
> >> + if (nor->spimem) {
> >> + struct spi_mem_op op =
> >> + SPI_NOR_READID_OP(naddr, ndummy, id, SPI_NOR_MAX_ID_LEN);
> >> +
> >> + spi_nor_spimem_setup_op(nor, &op, reg_proto);
> >> + ret = spi_mem_exec_op(nor->spimem, &op);
> >> + } else {
> >> + ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id,
> >> + SPI_NOR_MAX_ID_LEN);
> >> + }
> >> +
> >> + if (ret)
> >> + dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret);
> >
> > I think this message should be in spi_nor_detect(). Let octal DTR enable
>
> As of now every SPI NOR operation that return an error also prints a dbg
> message. I like this because it offers a smaller granularity on the error
> cause.
Yes, but I think this message would be misleading. If someone sees
"error reading JEDEC ID", they would think flash detection itself has
failed, not that we failed to switch to Octal DTR mode.
>
> > methods print their own, more specific error messages.
>
> How about duplicating the error in the octal dtr enable methods if you
> feel it is worth it?
They should at the very least explain that reading ID failed _after_
attempting to switch to Octal DTR. But I think it would just be simpler
if this is not printed here and the caller has the flexibility to
explain the error.
>
> >
> >> +
> >> + return ret;
> >> +}
> >> +
> >> /**
> >> * spi_nor_read_sr() - Read the Status Register.
> >> * @nor: pointer to 'struct spi_nor'.
> >> @@ -1649,28 +1684,15 @@ static const struct flash_info *spi_nor_match_id(struct spi_nor *nor,
> >> return NULL;
> >> }
> >>
> >> -static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
> >> +static const struct flash_info *spi_nor_detect(struct spi_nor *nor)
> >> {
> >> const struct flash_info *info;
> >> u8 *id = nor->bouncebuf;
> >> int ret;
> >>
> >> - if (nor->spimem) {
> >> - struct spi_mem_op op =
> >> - SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1),
> >> - SPI_MEM_OP_NO_ADDR,
> >> - SPI_MEM_OP_NO_DUMMY,
> >> - SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, id, 1));
> >> -
> >> - ret = spi_mem_exec_op(nor->spimem, &op);
> >> - } else {
> >> - ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id,
> >> - SPI_NOR_MAX_ID_LEN);
> >> - }
> >> - if (ret) {
> >> - dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret);
> >> + ret = spi_nor_read_id(nor, 0, 0, id, nor->reg_proto);
> >
> > Hmm, I wonder if it is better to explicitly use SNOR_PROTO_1_1_1 so
> > clearly signify that this is intended to use 1S-1S-1S only. What do you
> > think?
>
> I would keep it as it is for now, because it offers flexibility.
> If we ever gonna determine the protocol at runtime this will come in handy
> because it will work without touching the code. JESD216 suggests an algorithm
> that tries to determine the mode depending on the SFDP signature.
I was thinking exactly this but came to the opposite conclusion ;-). I
think this would imply that other protocols can be used to detect the
flash which is not true.
But I have no strong preferences here. Either is fine by me.
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
On 28/02/22 01:17PM, Tudor Ambarus wrote:
> Similar to spi_nor_match_name() extend the search of flash_info through
> all the manufacturers, this time doing the match by ID. There's no reason
> to limit the search per manufacturer yet, do it globally, search the flash
> in all the parts of all manufacturers in a single method.
>
> Signed-off-by: Tudor Ambarus <[email protected]>
Reviewed-by: Pratyush Yadav <[email protected]>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Am 2022-02-28 12:17, schrieb Tudor Ambarus:
> Similar to spi_nor_match_name() extend the search of flash_info through
> all the manufacturers, this time doing the match by ID. There's no
> reason
> to limit the search per manufacturer yet, do it globally, search the
> flash
> in all the parts of all manufacturers in a single method.
>
> Signed-off-by: Tudor Ambarus <[email protected]>
Reviewed-by: Michael Walle <[email protected]>
> ---
> drivers/mtd/spi-nor/core.c | 40 ++++++++++++++++++--------------------
> 1 file changed, 19 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index f3c359d03163..f87cb7d3daab 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1629,16 +1629,21 @@ static const struct spi_nor_manufacturer
> *manufacturers[] = {
> &spi_nor_xmc,
> };
>
> -static const struct flash_info *
> -spi_nor_search_part_by_id(const struct flash_info *parts, unsigned int
> nparts,
> - const u8 *id)
> +static const struct flash_info *spi_nor_match_id(struct spi_nor *nor,
> + const u8 *id)
> {
> - unsigned int i;
> + const struct flash_info *part;
> + unsigned int i, j;
>
> - for (i = 0; i < nparts; i++) {
> - if (parts[i].id_len &&
> - !memcmp(parts[i].id, id, parts[i].id_len))
> - return &parts[i];
> + for (i = 0; i < ARRAY_SIZE(manufacturers); i++) {
> + for (j = 0; j < manufacturers[i]->nparts; j++) {
> + part = &manufacturers[i]->parts[j];
> + if (part->id_len &&
> + !memcmp(part->id, id, part->id_len)) {
> + nor->manufacturer = manufacturers[i];
> + return part;
> + }
> + }
> }
>
> return NULL;
> @@ -1648,7 +1653,6 @@ static const struct flash_info
> *spi_nor_read_id(struct spi_nor *nor)
> {
> const struct flash_info *info;
> u8 *id = nor->bouncebuf;
> - unsigned int i;
> int ret;
>
> if (nor->spimem) {
> @@ -1668,19 +1672,13 @@ static const struct flash_info
> *spi_nor_read_id(struct spi_nor *nor)
> return ERR_PTR(ret);
> }
>
> - for (i = 0; i < ARRAY_SIZE(manufacturers); i++) {
> - info = spi_nor_search_part_by_id(manufacturers[i]->parts,
> - manufacturers[i]->nparts,
> - id);
> - if (info) {
> - nor->manufacturer = manufacturers[i];
> - return info;
> - }
> + info = spi_nor_match_id(nor, id);
> + if (!info) {
> + dev_err(nor->dev, "unrecognized JEDEC id bytes: %*ph\n",
> + SPI_NOR_MAX_ID_LEN, id);
> + return ERR_PTR(-ENODEV);
> }
> -
> - dev_err(nor->dev, "unrecognized JEDEC id bytes: %*ph\n",
> - SPI_NOR_MAX_ID_LEN, id);
> - return ERR_PTR(-ENODEV);
> + return info;
> }
>
> static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
--
-michael
Am 2022-03-21 18:42, schrieb Pratyush Yadav:
> On 21/03/22 12:50PM, [email protected] wrote:
>> On 3/21/22 14:14, Pratyush Yadav wrote:
>> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>> >
>> > On 28/02/22 01:17PM, Tudor Ambarus wrote:
>> >> In case spi_nor_match_name() returned NULL, the auto detection was
>> >> issued twice. There's no reason to try to detect the same chip twice,
>> >> do the auto detection only once.
>> >>
>> >> Signed-off-by: Tudor Ambarus <[email protected]>
>> >> ---
>> >> drivers/mtd/spi-nor/core.c | 10 ++++++----
>> >> 1 file changed, 6 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> >> index f87cb7d3daab..b1d6fa65417d 100644
>> >> --- a/drivers/mtd/spi-nor/core.c
>> >> +++ b/drivers/mtd/spi-nor/core.c
>> >> @@ -2894,13 +2894,15 @@ static const struct flash_info *spi_nor_match_name(struct spi_nor *nor,
>> >> static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
>> >> const char *name)
>> >> {
>> >> - const struct flash_info *info = NULL;
>> >> + const struct flash_info *info = NULL, *detected_info = NULL;
>> >>
>> >> if (name)
>> >> info = spi_nor_match_name(nor, name);
>> >> /* Try to auto-detect if chip name wasn't specified or not found */
>> >> - if (!info)
>> >> - info = spi_nor_read_id(nor);
>> >> + if (!info) {
>> >> + detected_info = spi_nor_read_id(nor);
>> >> + info = detected_info;
>> >> + }
>> >> if (IS_ERR_OR_NULL(info))
>> >> return ERR_PTR(-ENOENT);
>> >>
>> >> @@ -2908,7 +2910,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
>> >> * If caller has specified name of flash model that can normally be
>> >> * detected using JEDEC, let's verify it.
>> >> */
>> >> - if (name && info->id_len) {
>> >> + if (name && !detected_info && info->id_len) {
>> >> const struct flash_info *jinfo;
>> >>
>> >> jinfo = spi_nor_read_id(nor);
>> >
>> > I think the flow can be a little bit better. How about:
>> >
>> > if (name)
>> > info = spi_nor_match_name();
>> >
>> > if (!info) {
>> > info = spi_nor_read_id();
>> > if (IS_ERR_OR_NULL(info))
>> > return ERR_PTR(-ENOENT);
>> >
>> > return info;
>> > }
+1 for the flow. But is it correct that we just ignore any former
error and just replace it with ENOENT? Should we return NULL here
and let the caller handle the translation from NULL to ENOENT (and
keeping any other errors)
>>
>> Here we miss the IS_ERR check in case info is retrieved with
>> spi_nor_match_name().
>> Do you expect spi_nor_match_name() to ever return an error? As it is
>> now it doesn't.
>> I'm fine either way. In case you want me to follow your suggestion,
>> give me a sign
>> and I'll make a dedicated patch to move the IS_ERR_OR_NULL check. Will
>> add your
>> Suggested-by tag.
>
> I think it should be safe to assume it won't ever return an error since
> all it does is iterate over an array that is always present. I don't
> see
> that changing in the foreseeable future either. So I think not having
> the IS_ERR check is fine.
But what does it cost to just add the error check now so it won't
be forgotten in the future?
if (name) {
info = spi_nor_match_name();
if (IS_ERR(info))
return info;
}
if (!info)
return spi_nor_read_id();
<flash model check code follows here>
And then let the caller handle NULL and translate it to ENOENT.
-michael
Am 2022-02-28 12:17, schrieb Tudor Ambarus:
> RDID is used in the core to auto detect the flash, but also by some
> manufacturer drivers that contain flashes that support Octal DTR mode,
> so that they can read the flash ID after the switch to Octal DTR was
> made
> to test if the switch was successful. Introduce a core method for RDID
> op
> to avoid code duplication.
Some or all? Is that specific to the flash or can we just check that
readid works in spi_nor_octal_dtr_enable()? That way we could also
just get rid of the proto parameter for the read_id because it can
be called after we set the reg_proto.
-michael
>
> Signed-off-by: Tudor Ambarus <[email protected]>
> ---
> drivers/mtd/spi-nor/core.c | 58 ++++++++++++++++++++++++++------------
> drivers/mtd/spi-nor/core.h | 9 ++++++
> 2 files changed, 49 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index b1d6fa65417d..281e3d25f74c 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -369,6 +369,41 @@ int spi_nor_write_disable(struct spi_nor *nor)
> return ret;
> }
>
> +/**
> + * spi_nor_read_id() - Read the JEDEC ID.
> + * @nor: pointer to 'struct spi_nor'.
> + * @naddr: number of address bytes to send. Can be zero if the
> operation
> + * does not need to send an address.
> + * @ndummy: number of dummy bytes to send after an opcode or address.
> Can
> + * be zero if the operation does not require dummy bytes.
> + * @id: pointer to a DMA-able buffer where the value of the JEDEC ID
> + * will be written.
> + * @reg_proto: the SPI protocol for register operation.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id,
> + enum spi_nor_protocol reg_proto)
> +{
> + int ret;
> +
> + if (nor->spimem) {
> + struct spi_mem_op op =
> + SPI_NOR_READID_OP(naddr, ndummy, id, SPI_NOR_MAX_ID_LEN);
> +
> + spi_nor_spimem_setup_op(nor, &op, reg_proto);
> + ret = spi_mem_exec_op(nor->spimem, &op);
> + } else {
> + ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id,
> + SPI_NOR_MAX_ID_LEN);
> + }
> +
> + if (ret)
> + dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret);
> +
> + return ret;
> +}
> +
> /**
> * spi_nor_read_sr() - Read the Status Register.
> * @nor: pointer to 'struct spi_nor'.
> @@ -1649,28 +1684,15 @@ static const struct flash_info
> *spi_nor_match_id(struct spi_nor *nor,
> return NULL;
> }
>
> -static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
> +static const struct flash_info *spi_nor_detect(struct spi_nor *nor)
> {
> const struct flash_info *info;
> u8 *id = nor->bouncebuf;
> int ret;
>
> - if (nor->spimem) {
> - struct spi_mem_op op =
> - SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1),
> - SPI_MEM_OP_NO_ADDR,
> - SPI_MEM_OP_NO_DUMMY,
> - SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, id, 1));
> -
> - ret = spi_mem_exec_op(nor->spimem, &op);
> - } else {
> - ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id,
> - SPI_NOR_MAX_ID_LEN);
> - }
> - if (ret) {
> - dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret);
> + ret = spi_nor_read_id(nor, 0, 0, id, nor->reg_proto);
> + if (ret)
> return ERR_PTR(ret);
> - }
>
> info = spi_nor_match_id(nor, id);
> if (!info) {
> @@ -2900,7 +2922,7 @@ static const struct flash_info
> *spi_nor_get_flash_info(struct spi_nor *nor,
> info = spi_nor_match_name(nor, name);
> /* Try to auto-detect if chip name wasn't specified or not found */
> if (!info) {
> - detected_info = spi_nor_read_id(nor);
> + detected_info = spi_nor_detect(nor);
> info = detected_info;
> }
> if (IS_ERR_OR_NULL(info))
> @@ -2913,7 +2935,7 @@ static const struct flash_info
> *spi_nor_get_flash_info(struct spi_nor *nor,
> if (name && !detected_info && info->id_len) {
> const struct flash_info *jinfo;
>
> - jinfo = spi_nor_read_id(nor);
> + jinfo = spi_nor_detect(nor);
> if (IS_ERR(jinfo)) {
> return jinfo;
> } else if (jinfo != info) {
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index b7fd760e3b47..f952061d5c24 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -11,6 +11,13 @@
>
> #define SPI_NOR_MAX_ID_LEN 6
>
> +/* Standard SPI NOR flash operations. */
> +#define SPI_NOR_READID_OP(naddr, ndummy, buf, len) \
> + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 0), \
> + SPI_MEM_OP_ADDR(naddr, 0, 0), \
> + SPI_MEM_OP_DUMMY(ndummy, 0), \
> + SPI_MEM_OP_DATA_IN(len, buf, 0))
> +
> enum spi_nor_option_flags {
> SNOR_F_HAS_SR_TB = BIT(0),
> SNOR_F_NO_OP_CHIP_ERASE = BIT(1),
> @@ -534,6 +541,8 @@ void spi_nor_unlock_and_unprep(struct spi_nor
> *nor);
> int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor);
> int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor);
> int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor);
> +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id,
> + enum spi_nor_protocol reg_proto);
> int spi_nor_read_sr(struct spi_nor *nor, u8 *sr);
> int spi_nor_sr_ready(struct spi_nor *nor);
> int spi_nor_read_cr(struct spi_nor *nor, u8 *cr);
--
-michael
Am 2022-02-28 12:17, schrieb Tudor Ambarus:
> There are manufacturers that use registers indexed by address. Some of
> them support "read/write any register" opcodes. Provide core methods
> that
> can be used by all manufacturers. SPI NOR controller ops are
> intentionally
> not supported as we intend to move all the SPI NOR controller drivers
> under the SPI subsystem.
>
> Signed-off-by: Tudor Ambarus <[email protected]>
> Tested-by: Takahiro Kuwano <[email protected]>
> Reviewed-by: Pratyush Yadav <[email protected]>
> ---
> drivers/mtd/spi-nor/core.c | 41 ++++++++++++++++++++++++++++++++++++++
> drivers/mtd/spi-nor/core.h | 4 ++++
> 2 files changed, 45 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 281e3d25f74c..f1aa1e2ea7c9 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -307,6 +307,47 @@ ssize_t spi_nor_write_data(struct spi_nor *nor,
> loff_t to, size_t len,
> return nor->controller_ops->write(nor, to, len, buf);
> }
>
> +/**
> + * spi_nor_read_reg() - read register to flash memory
> + * @nor: pointer to 'struct spi_nor'.
> + * @op: SPI memory operation. op->data.buf must be DMA-able.
> + * @proto: SPI protocol to use for the register operation.
> + *
> + * Return: zero on success, -errno otherwise
> + */
> +int spi_nor_read_reg(struct spi_nor *nor, struct spi_mem_op *op,
> + enum spi_nor_protocol proto)
> +{
> + if (!nor->spimem)
> + return -EOPNOTSUPP;
> +
> + spi_nor_spimem_setup_op(nor, op, proto);
> + return spi_nor_spimem_exec_op(nor, op);
> +}
> +
> +/**
> + * spi_nor_write_reg() - write register to flash memory
> + * @nor: pointer to 'struct spi_nor'
> + * @op: SPI memory operation. op->data.buf must be DMA-able.
> + * @proto: SPI protocol to use for the register operation.
> + *
> + * Return: zero on success, -errno otherwise
> + */
> +int spi_nor_write_reg(struct spi_nor *nor, struct spi_mem_op *op,
> + enum spi_nor_protocol proto)
> +{
> + int ret;
> +
> + if (!nor->spimem)
> + return -EOPNOTSUPP;
> +
> + ret = spi_nor_write_enable(nor);
> + if (ret)
> + return ret;
> + spi_nor_spimem_setup_op(nor, op, proto);
> + return spi_nor_spimem_exec_op(nor, op);
> +}
> +
> /**
> * spi_nor_write_enable() - Set write enable latch with Write Enable
> command.
> * @nor: pointer to 'struct spi_nor'.
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index f952061d5c24..7c704475946d 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -554,6 +554,10 @@ ssize_t spi_nor_read_data(struct spi_nor *nor,
> loff_t from, size_t len,
> u8 *buf);
> ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
> const u8 *buf);
> +int spi_nor_read_reg(struct spi_nor *nor, struct spi_mem_op *op,
> + enum spi_nor_protocol proto);
> +int spi_nor_write_reg(struct spi_nor *nor, struct spi_mem_op *op,
> + enum spi_nor_protocol proto);
These look rather odd. I'd expect to see an address and such for
such a "random register read/write". Looks like these functions
don't do much except calling spi_nor_spimem_setup_op() and
exec_op() and don't have anything to do with register access
(except maybe for the write enable). Can't we have a bit more
sophisticated interface in the core? Something that calls into
the flash driver to assemble the spi_mem_op automatically? Assuming
that this will be used more often to access registers in a flash.
-michael
> int spi_nor_erase_sector(struct spi_nor *nor, u32 addr);
>
> int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t
> len, u8 *buf);
On 21/03/22 11:56PM, Michael Walle wrote:
> Am 2022-02-28 12:17, schrieb Tudor Ambarus:
> > RDID is used in the core to auto detect the flash, but also by some
> > manufacturer drivers that contain flashes that support Octal DTR mode,
> > so that they can read the flash ID after the switch to Octal DTR was
> > made
> > to test if the switch was successful. Introduce a core method for RDID
> > op
> > to avoid code duplication.
>
> Some or all? Is that specific to the flash or can we just check that
> readid works in spi_nor_octal_dtr_enable()? That way we could also
> just get rid of the proto parameter for the read_id because it can
> be called after we set the reg_proto.
It is specific to the flash. Not all flashes support RDID in 8D mode.
And the RDID command is also different in 8D mode for various flashes.
For example, Micron MT35XU512ABA flash expects 8 dummy cycles and 0
address cycles. Cypress S28HS512T expects 4 address cycles and 3 dummy
cycles.
The octal_dtr_enable hook would know what parameters to use but it is
harder for the core to know since this information is not discoverable
via SFDP.
>
> -michael
>
> >
> > Signed-off-by: Tudor Ambarus <[email protected]>
> > ---
> > drivers/mtd/spi-nor/core.c | 58 ++++++++++++++++++++++++++------------
> > drivers/mtd/spi-nor/core.h | 9 ++++++
> > 2 files changed, 49 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index b1d6fa65417d..281e3d25f74c 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -369,6 +369,41 @@ int spi_nor_write_disable(struct spi_nor *nor)
> > return ret;
> > }
> >
> > +/**
> > + * spi_nor_read_id() - Read the JEDEC ID.
> > + * @nor: pointer to 'struct spi_nor'.
> > + * @naddr: number of address bytes to send. Can be zero if the
> > operation
> > + * does not need to send an address.
> > + * @ndummy: number of dummy bytes to send after an opcode or address.
> > Can
> > + * be zero if the operation does not require dummy bytes.
> > + * @id: pointer to a DMA-able buffer where the value of the JEDEC ID
> > + * will be written.
> > + * @reg_proto: the SPI protocol for register operation.
> > + *
> > + * Return: 0 on success, -errno otherwise.
> > + */
> > +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id,
> > + enum spi_nor_protocol reg_proto)
> > +{
> > + int ret;
> > +
> > + if (nor->spimem) {
> > + struct spi_mem_op op =
> > + SPI_NOR_READID_OP(naddr, ndummy, id, SPI_NOR_MAX_ID_LEN);
> > +
> > + spi_nor_spimem_setup_op(nor, &op, reg_proto);
> > + ret = spi_mem_exec_op(nor->spimem, &op);
> > + } else {
> > + ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id,
> > + SPI_NOR_MAX_ID_LEN);
> > + }
> > +
> > + if (ret)
> > + dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret);
> > +
> > + return ret;
> > +}
> > +
> > /**
> > * spi_nor_read_sr() - Read the Status Register.
> > * @nor: pointer to 'struct spi_nor'.
> > @@ -1649,28 +1684,15 @@ static const struct flash_info
> > *spi_nor_match_id(struct spi_nor *nor,
> > return NULL;
> > }
> >
> > -static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
> > +static const struct flash_info *spi_nor_detect(struct spi_nor *nor)
> > {
> > const struct flash_info *info;
> > u8 *id = nor->bouncebuf;
> > int ret;
> >
> > - if (nor->spimem) {
> > - struct spi_mem_op op =
> > - SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1),
> > - SPI_MEM_OP_NO_ADDR,
> > - SPI_MEM_OP_NO_DUMMY,
> > - SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, id, 1));
> > -
> > - ret = spi_mem_exec_op(nor->spimem, &op);
> > - } else {
> > - ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id,
> > - SPI_NOR_MAX_ID_LEN);
> > - }
> > - if (ret) {
> > - dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret);
> > + ret = spi_nor_read_id(nor, 0, 0, id, nor->reg_proto);
> > + if (ret)
> > return ERR_PTR(ret);
> > - }
> >
> > info = spi_nor_match_id(nor, id);
> > if (!info) {
> > @@ -2900,7 +2922,7 @@ static const struct flash_info
> > *spi_nor_get_flash_info(struct spi_nor *nor,
> > info = spi_nor_match_name(nor, name);
> > /* Try to auto-detect if chip name wasn't specified or not found */
> > if (!info) {
> > - detected_info = spi_nor_read_id(nor);
> > + detected_info = spi_nor_detect(nor);
> > info = detected_info;
> > }
> > if (IS_ERR_OR_NULL(info))
> > @@ -2913,7 +2935,7 @@ static const struct flash_info
> > *spi_nor_get_flash_info(struct spi_nor *nor,
> > if (name && !detected_info && info->id_len) {
> > const struct flash_info *jinfo;
> >
> > - jinfo = spi_nor_read_id(nor);
> > + jinfo = spi_nor_detect(nor);
> > if (IS_ERR(jinfo)) {
> > return jinfo;
> > } else if (jinfo != info) {
> > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> > index b7fd760e3b47..f952061d5c24 100644
> > --- a/drivers/mtd/spi-nor/core.h
> > +++ b/drivers/mtd/spi-nor/core.h
> > @@ -11,6 +11,13 @@
> >
> > #define SPI_NOR_MAX_ID_LEN 6
> >
> > +/* Standard SPI NOR flash operations. */
> > +#define SPI_NOR_READID_OP(naddr, ndummy, buf, len) \
> > + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 0), \
> > + SPI_MEM_OP_ADDR(naddr, 0, 0), \
> > + SPI_MEM_OP_DUMMY(ndummy, 0), \
> > + SPI_MEM_OP_DATA_IN(len, buf, 0))
> > +
> > enum spi_nor_option_flags {
> > SNOR_F_HAS_SR_TB = BIT(0),
> > SNOR_F_NO_OP_CHIP_ERASE = BIT(1),
> > @@ -534,6 +541,8 @@ void spi_nor_unlock_and_unprep(struct spi_nor *nor);
> > int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor);
> > int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor);
> > int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor);
> > +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id,
> > + enum spi_nor_protocol reg_proto);
> > int spi_nor_read_sr(struct spi_nor *nor, u8 *sr);
> > int spi_nor_sr_ready(struct spi_nor *nor);
> > int spi_nor_read_cr(struct spi_nor *nor, u8 *cr);
>
> --
> -michael
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Am 2022-03-22 08:32, schrieb Pratyush Yadav:
> On 21/03/22 11:56PM, Michael Walle wrote:
>> Am 2022-02-28 12:17, schrieb Tudor Ambarus:
>> > RDID is used in the core to auto detect the flash, but also by some
>> > manufacturer drivers that contain flashes that support Octal DTR mode,
>> > so that they can read the flash ID after the switch to Octal DTR was
>> > made
>> > to test if the switch was successful. Introduce a core method for RDID
>> > op
>> > to avoid code duplication.
>>
>> Some or all? Is that specific to the flash or can we just check that
>> readid works in spi_nor_octal_dtr_enable()? That way we could also
>> just get rid of the proto parameter for the read_id because it can
>> be called after we set the reg_proto.
>
> It is specific to the flash. Not all flashes support RDID in 8D mode.
> And the RDID command is also different in 8D mode for various flashes.
> For example, Micron MT35XU512ABA flash expects 8 dummy cycles and 0
> address cycles. Cypress S28HS512T expects 4 address cycles and 3 dummy
> cycles.
>
> The octal_dtr_enable hook would know what parameters to use but it is
> harder for the core to know since this information is not discoverable
> via SFDP.
Ah, I see, thanks for clarification.
-michael
On 3/21/22 19:39, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 21/03/22 01:18PM, [email protected] wrote:
>> On 3/21/22 14:21, Pratyush Yadav wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 28/02/22 01:17PM, Tudor Ambarus wrote:
>>>> RDID is used in the core to auto detect the flash, but also by some
>>>> manufacturer drivers that contain flashes that support Octal DTR mode,
>>>> so that they can read the flash ID after the switch to Octal DTR was made
>>>> to test if the switch was successful. Introduce a core method for RDID op
>>>> to avoid code duplication.
>>>>
>>>> Signed-off-by: Tudor Ambarus <[email protected]>
>>>> ---
>>>> drivers/mtd/spi-nor/core.c | 58 ++++++++++++++++++++++++++------------
>>>> drivers/mtd/spi-nor/core.h | 9 ++++++
>>>> 2 files changed, 49 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>> index b1d6fa65417d..281e3d25f74c 100644
>>>> --- a/drivers/mtd/spi-nor/core.c
>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>> @@ -369,6 +369,41 @@ int spi_nor_write_disable(struct spi_nor *nor)
>>>> return ret;
>>>> }
>>>>
>>>> +/**
>>>> + * spi_nor_read_id() - Read the JEDEC ID.
>>>> + * @nor: pointer to 'struct spi_nor'.
>>>> + * @naddr: number of address bytes to send. Can be zero if the operation
>>>> + * does not need to send an address.
>>>> + * @ndummy: number of dummy bytes to send after an opcode or address. Can
>>>> + * be zero if the operation does not require dummy bytes.
>>>> + * @id: pointer to a DMA-able buffer where the value of the JEDEC ID
>>>> + * will be written.
>>>> + * @reg_proto: the SPI protocol for register operation.
>>>> + *
>>>> + * Return: 0 on success, -errno otherwise.
>>>> + */
>>>> +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id,
>>>> + enum spi_nor_protocol reg_proto)
>>>
>>> Nitpick: Could just call it 'proto'.
>>
>> sure, will update
>>
>>>
>>>> +{
>>>> + int ret;
>>>> +
>>>> + if (nor->spimem) {
>>>> + struct spi_mem_op op =
>>>> + SPI_NOR_READID_OP(naddr, ndummy, id, SPI_NOR_MAX_ID_LEN);
>>>> +
>>>> + spi_nor_spimem_setup_op(nor, &op, reg_proto);
>>>> + ret = spi_mem_exec_op(nor->spimem, &op);
>>>> + } else {
>>>> + ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id,
>>>> + SPI_NOR_MAX_ID_LEN);
>>>> + }
>>>> +
>>>> + if (ret)
>>>> + dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret);
>>>
>>> I think this message should be in spi_nor_detect(). Let octal DTR enable
>>
>> As of now every SPI NOR operation that return an error also prints a dbg
>> message. I like this because it offers a smaller granularity on the error
>> cause.
>
> Yes, but I think this message would be misleading. If someone sees
> "error reading JEDEC ID", they would think flash detection itself has
> failed, not that we failed to switch to Octal DTR mode.
>
>>
>>> methods print their own, more specific error messages.
>>
>> How about duplicating the error in the octal dtr enable methods if you
>> feel it is worth it?
>
> They should at the very least explain that reading ID failed _after_
> attempting to switch to Octal DTR. But I think it would just be simpler
> if this is not printed here and the caller has the flexibility to
> explain the error.
If the first readID fails, the one that identifies the flash, then the
octal dtr will not be run, thus a single error message. When octal dtr
fails, 2 errors can be printed, one specifying what failed (the read ID
command) and the second where it failed (at the octal dtr enable method).
But I don't care too much, I'll follow your suggestion.
>
>>
>>>
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> /**
>>>> * spi_nor_read_sr() - Read the Status Register.
>>>> * @nor: pointer to 'struct spi_nor'.
>>>> @@ -1649,28 +1684,15 @@ static const struct flash_info *spi_nor_match_id(struct spi_nor *nor,
>>>> return NULL;
>>>> }
>>>>
>>>> -static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
>>>> +static const struct flash_info *spi_nor_detect(struct spi_nor *nor)
>>>> {
>>>> const struct flash_info *info;
>>>> u8 *id = nor->bouncebuf;
>>>> int ret;
>>>>
>>>> - if (nor->spimem) {
>>>> - struct spi_mem_op op =
>>>> - SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1),
>>>> - SPI_MEM_OP_NO_ADDR,
>>>> - SPI_MEM_OP_NO_DUMMY,
>>>> - SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, id, 1));
>>>> -
>>>> - ret = spi_mem_exec_op(nor->spimem, &op);
>>>> - } else {
>>>> - ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id,
>>>> - SPI_NOR_MAX_ID_LEN);
>>>> - }
>>>> - if (ret) {
>>>> - dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret);
>>>> + ret = spi_nor_read_id(nor, 0, 0, id, nor->reg_proto);
>>>
>>> Hmm, I wonder if it is better to explicitly use SNOR_PROTO_1_1_1 so
>>> clearly signify that this is intended to use 1S-1S-1S only. What do you
>>> think?
>>
>> I would keep it as it is for now, because it offers flexibility.
>> If we ever gonna determine the protocol at runtime this will come in handy
>> because it will work without touching the code. JESD216 suggests an algorithm
>> that tries to determine the mode depending on the SFDP signature.
>
> I was thinking exactly this but came to the opposite conclusion ;-). I
> think this would imply that other protocols can be used to detect the
> flash which is not true.
It can become true. As you already specified 8d-8d-8d is supported by some flashes
and we can implement hooks for their specific 8d-8d-8d readID command. The logic
will complicate a bit as one has to adjust the hwcaps before issuing the 8d-8d-8d
readID, but it's doable. Otherwise, if the bootloaders pass you the flash in octal
dtr mode, you'll have to disable it, issue readID is 1-1-1 and then re-enable it.
>
> But I have no strong preferences here. Either is fine by me.
I don't have strong preferences either, but it seems that there's room for discussion
on this, so I would keep it for later. Is that fine?
I can add a comment if you prefer, specifying that at this point nor->reg_proto is in
1-1-1 mode.
Cheers,
ta
On 30/03/22 06:53AM, [email protected] wrote:
> On 3/21/22 19:39, Pratyush Yadav wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > On 21/03/22 01:18PM, [email protected] wrote:
> >> On 3/21/22 14:21, Pratyush Yadav wrote:
> >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>>
> >>> On 28/02/22 01:17PM, Tudor Ambarus wrote:
> >>>> RDID is used in the core to auto detect the flash, but also by some
> >>>> manufacturer drivers that contain flashes that support Octal DTR mode,
> >>>> so that they can read the flash ID after the switch to Octal DTR was made
> >>>> to test if the switch was successful. Introduce a core method for RDID op
> >>>> to avoid code duplication.
> >>>>
> >>>> Signed-off-by: Tudor Ambarus <[email protected]>
> >>>> ---
> >>>> drivers/mtd/spi-nor/core.c | 58 ++++++++++++++++++++++++++------------
> >>>> drivers/mtd/spi-nor/core.h | 9 ++++++
> >>>> 2 files changed, 49 insertions(+), 18 deletions(-)
> >>>>
> >>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> >>>> index b1d6fa65417d..281e3d25f74c 100644
> >>>> --- a/drivers/mtd/spi-nor/core.c
> >>>> +++ b/drivers/mtd/spi-nor/core.c
> >>>> @@ -369,6 +369,41 @@ int spi_nor_write_disable(struct spi_nor *nor)
> >>>> return ret;
> >>>> }
> >>>>
> >>>> +/**
> >>>> + * spi_nor_read_id() - Read the JEDEC ID.
> >>>> + * @nor: pointer to 'struct spi_nor'.
> >>>> + * @naddr: number of address bytes to send. Can be zero if the operation
> >>>> + * does not need to send an address.
> >>>> + * @ndummy: number of dummy bytes to send after an opcode or address. Can
> >>>> + * be zero if the operation does not require dummy bytes.
> >>>> + * @id: pointer to a DMA-able buffer where the value of the JEDEC ID
> >>>> + * will be written.
> >>>> + * @reg_proto: the SPI protocol for register operation.
> >>>> + *
> >>>> + * Return: 0 on success, -errno otherwise.
> >>>> + */
> >>>> +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id,
> >>>> + enum spi_nor_protocol reg_proto)
> >>>
> >>> Nitpick: Could just call it 'proto'.
> >>
> >> sure, will update
> >>
> >>>
> >>>> +{
> >>>> + int ret;
> >>>> +
> >>>> + if (nor->spimem) {
> >>>> + struct spi_mem_op op =
> >>>> + SPI_NOR_READID_OP(naddr, ndummy, id, SPI_NOR_MAX_ID_LEN);
> >>>> +
> >>>> + spi_nor_spimem_setup_op(nor, &op, reg_proto);
> >>>> + ret = spi_mem_exec_op(nor->spimem, &op);
> >>>> + } else {
> >>>> + ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id,
> >>>> + SPI_NOR_MAX_ID_LEN);
> >>>> + }
> >>>> +
> >>>> + if (ret)
> >>>> + dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret);
> >>>
> >>> I think this message should be in spi_nor_detect(). Let octal DTR enable
> >>
> >> As of now every SPI NOR operation that return an error also prints a dbg
> >> message. I like this because it offers a smaller granularity on the error
> >> cause.
> >
> > Yes, but I think this message would be misleading. If someone sees
> > "error reading JEDEC ID", they would think flash detection itself has
> > failed, not that we failed to switch to Octal DTR mode.
> >
> >>
> >>> methods print their own, more specific error messages.
> >>
> >> How about duplicating the error in the octal dtr enable methods if you
> >> feel it is worth it?
> >
> > They should at the very least explain that reading ID failed _after_
> > attempting to switch to Octal DTR. But I think it would just be simpler
> > if this is not printed here and the caller has the flexibility to
> > explain the error.
>
> If the first readID fails, the one that identifies the flash, then the
> octal dtr will not be run, thus a single error message. When octal dtr
> fails, 2 errors can be printed, one specifying what failed (the read ID
> command) and the second where it failed (at the octal dtr enable method).
> But I don't care too much, I'll follow your suggestion.
>
> >
> >>
> >>>
> >>>> +
> >>>> + return ret;
> >>>> +}
> >>>> +
> >>>> /**
> >>>> * spi_nor_read_sr() - Read the Status Register.
> >>>> * @nor: pointer to 'struct spi_nor'.
> >>>> @@ -1649,28 +1684,15 @@ static const struct flash_info *spi_nor_match_id(struct spi_nor *nor,
> >>>> return NULL;
> >>>> }
> >>>>
> >>>> -static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
> >>>> +static const struct flash_info *spi_nor_detect(struct spi_nor *nor)
> >>>> {
> >>>> const struct flash_info *info;
> >>>> u8 *id = nor->bouncebuf;
> >>>> int ret;
> >>>>
> >>>> - if (nor->spimem) {
> >>>> - struct spi_mem_op op =
> >>>> - SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1),
> >>>> - SPI_MEM_OP_NO_ADDR,
> >>>> - SPI_MEM_OP_NO_DUMMY,
> >>>> - SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, id, 1));
> >>>> -
> >>>> - ret = spi_mem_exec_op(nor->spimem, &op);
> >>>> - } else {
> >>>> - ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id,
> >>>> - SPI_NOR_MAX_ID_LEN);
> >>>> - }
> >>>> - if (ret) {
> >>>> - dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret);
> >>>> + ret = spi_nor_read_id(nor, 0, 0, id, nor->reg_proto);
> >>>
> >>> Hmm, I wonder if it is better to explicitly use SNOR_PROTO_1_1_1 so
> >>> clearly signify that this is intended to use 1S-1S-1S only. What do you
> >>> think?
> >>
> >> I would keep it as it is for now, because it offers flexibility.
> >> If we ever gonna determine the protocol at runtime this will come in handy
> >> because it will work without touching the code. JESD216 suggests an algorithm
> >> that tries to determine the mode depending on the SFDP signature.
> >
> > I was thinking exactly this but came to the opposite conclusion ;-). I
> > think this would imply that other protocols can be used to detect the
> > flash which is not true.
>
> It can become true. As you already specified 8d-8d-8d is supported by some flashes
> and we can implement hooks for their specific 8d-8d-8d readID command. The logic
> will complicate a bit as one has to adjust the hwcaps before issuing the 8d-8d-8d
> readID, but it's doable. Otherwise, if the bootloaders pass you the flash in octal
> dtr mode, you'll have to disable it, issue readID is 1-1-1 and then re-enable it.
Right.
>
> >
> > But I have no strong preferences here. Either is fine by me.
>
> I don't have strong preferences either, but it seems that there's room for discussion
> on this, so I would keep it for later. Is that fine?
Fine by me. It should be fine without the comment. It is not too hard to
see what nor->reg_proto is initialized to.
> I can add a comment if you prefer, specifying that at this point nor->reg_proto is in
> 1-1-1 mode.
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
On 21/03/22 11:38PM, Michael Walle wrote:
> Am 2022-03-21 18:42, schrieb Pratyush Yadav:
> > On 21/03/22 12:50PM, [email protected] wrote:
> > > On 3/21/22 14:14, Pratyush Yadav wrote:
> > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > > >
> > > > On 28/02/22 01:17PM, Tudor Ambarus wrote:
> > > >> In case spi_nor_match_name() returned NULL, the auto detection was
> > > >> issued twice. There's no reason to try to detect the same chip twice,
> > > >> do the auto detection only once.
> > > >>
> > > >> Signed-off-by: Tudor Ambarus <[email protected]>
> > > >> ---
> > > >> drivers/mtd/spi-nor/core.c | 10 ++++++----
> > > >> 1 file changed, 6 insertions(+), 4 deletions(-)
> > > >>
> > > >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > > >> index f87cb7d3daab..b1d6fa65417d 100644
> > > >> --- a/drivers/mtd/spi-nor/core.c
> > > >> +++ b/drivers/mtd/spi-nor/core.c
> > > >> @@ -2894,13 +2894,15 @@ static const struct flash_info *spi_nor_match_name(struct spi_nor *nor,
> > > >> static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
> > > >> const char *name)
> > > >> {
> > > >> - const struct flash_info *info = NULL;
> > > >> + const struct flash_info *info = NULL, *detected_info = NULL;
> > > >>
> > > >> if (name)
> > > >> info = spi_nor_match_name(nor, name);
> > > >> /* Try to auto-detect if chip name wasn't specified or not found */
> > > >> - if (!info)
> > > >> - info = spi_nor_read_id(nor);
> > > >> + if (!info) {
> > > >> + detected_info = spi_nor_read_id(nor);
> > > >> + info = detected_info;
> > > >> + }
> > > >> if (IS_ERR_OR_NULL(info))
> > > >> return ERR_PTR(-ENOENT);
> > > >>
> > > >> @@ -2908,7 +2910,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
> > > >> * If caller has specified name of flash model that can normally be
> > > >> * detected using JEDEC, let's verify it.
> > > >> */
> > > >> - if (name && info->id_len) {
> > > >> + if (name && !detected_info && info->id_len) {
> > > >> const struct flash_info *jinfo;
> > > >>
> > > >> jinfo = spi_nor_read_id(nor);
> > > >
> > > > I think the flow can be a little bit better. How about:
> > > >
> > > > if (name)
> > > > info = spi_nor_match_name();
> > > >
> > > > if (!info) {
> > > > info = spi_nor_read_id();
> > > > if (IS_ERR_OR_NULL(info))
> > > > return ERR_PTR(-ENOENT);
> > > >
> > > > return info;
> > > > }
>
> +1 for the flow. But is it correct that we just ignore any former
> error and just replace it with ENOENT? Should we return NULL here
> and let the caller handle the translation from NULL to ENOENT (and
> keeping any other errors)
>
> > >
> > > Here we miss the IS_ERR check in case info is retrieved with
> > > spi_nor_match_name().
> > > Do you expect spi_nor_match_name() to ever return an error? As it is
> > > now it doesn't.
> > > I'm fine either way. In case you want me to follow your suggestion,
> > > give me a sign
> > > and I'll make a dedicated patch to move the IS_ERR_OR_NULL check.
> > > Will add your
> > > Suggested-by tag.
> >
> > I think it should be safe to assume it won't ever return an error since
> > all it does is iterate over an array that is always present. I don't see
> > that changing in the foreseeable future either. So I think not having
> > the IS_ERR check is fine.
>
> But what does it cost to just add the error check now so it won't
> be forgotten in the future?
>
> if (name) {
> info = spi_nor_match_name();
> if (IS_ERR(info))
> return info;
> }
> if (!info)
> return spi_nor_read_id();
>
> <flash model check code follows here>
>
> And then let the caller handle NULL and translate it to ENOENT.
Sounds good to me.
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
On 3/22/22 01:13, Michael Walle wrote:
> Am 2022-02-28 12:17, schrieb Tudor Ambarus:
>> There are manufacturers that use registers indexed by address. Some of
>> them support "read/write any register" opcodes. Provide core methods that
>> can be used by all manufacturers. SPI NOR controller ops are intentionally
>> not supported as we intend to move all the SPI NOR controller drivers
>> under the SPI subsystem.
>>
>> Signed-off-by: Tudor Ambarus <[email protected]>
>> Tested-by: Takahiro Kuwano <[email protected]>
>> Reviewed-by: Pratyush Yadav <[email protected]>
>> ---
>> drivers/mtd/spi-nor/core.c | 41 ++++++++++++++++++++++++++++++++++++++
>> drivers/mtd/spi-nor/core.h | 4 ++++
>> 2 files changed, 45 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 281e3d25f74c..f1aa1e2ea7c9 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -307,6 +307,47 @@ ssize_t spi_nor_write_data(struct spi_nor *nor,
>> loff_t to, size_t len,
>> return nor->controller_ops->write(nor, to, len, buf);
>> }
>>
>> +/**
>> + * spi_nor_read_reg() - read register to flash memory
>> + * @nor: pointer to 'struct spi_nor'.
>> + * @op: SPI memory operation. op->data.buf must be DMA-able.
>> + * @proto: SPI protocol to use for the register operation.
>> + *
>> + * Return: zero on success, -errno otherwise
>> + */
>> +int spi_nor_read_reg(struct spi_nor *nor, struct spi_mem_op *op,
>> + enum spi_nor_protocol proto)
>> +{
>> + if (!nor->spimem)
>> + return -EOPNOTSUPP;
>> +
>> + spi_nor_spimem_setup_op(nor, op, proto);
>> + return spi_nor_spimem_exec_op(nor, op);
>> +}
>> +
>> +/**
>> + * spi_nor_write_reg() - write register to flash memory
>> + * @nor: pointer to 'struct spi_nor'
>> + * @op: SPI memory operation. op->data.buf must be DMA-able.
>> + * @proto: SPI protocol to use for the register operation.
>> + *
>> + * Return: zero on success, -errno otherwise
>> + */
>> +int spi_nor_write_reg(struct spi_nor *nor, struct spi_mem_op *op,
>> + enum spi_nor_protocol proto)
>> +{
>> + int ret;
>> +
>> + if (!nor->spimem)
>> + return -EOPNOTSUPP;
>> +
>> + ret = spi_nor_write_enable(nor);
>> + if (ret)
>> + return ret;
>> + spi_nor_spimem_setup_op(nor, op, proto);
>> + return spi_nor_spimem_exec_op(nor, op);
>> +}
>> +
>> /**
>> * spi_nor_write_enable() - Set write enable latch with Write Enable command.
>> * @nor: pointer to 'struct spi_nor'.
>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>> index f952061d5c24..7c704475946d 100644
>> --- a/drivers/mtd/spi-nor/core.h
>> +++ b/drivers/mtd/spi-nor/core.h
>> @@ -554,6 +554,10 @@ ssize_t spi_nor_read_data(struct spi_nor *nor,
>> loff_t from, size_t len,
>> u8 *buf);
>> ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
>> const u8 *buf);
>> +int spi_nor_read_reg(struct spi_nor *nor, struct spi_mem_op *op,
>> + enum spi_nor_protocol proto);
>> +int spi_nor_write_reg(struct spi_nor *nor, struct spi_mem_op *op,
>> + enum spi_nor_protocol proto);
>
> These look rather odd. I'd expect to see an address and such for
> such a "random register read/write". Looks like these functions
There are at least 6 function parameters if we want to explicitly pass function
arguments: pointer to nor, opcode, addr.nbytes, addr, buf.nbytes, buf. Not to
mention ndummy for reads. A bit too much for me, so I preferred passing spi_mem_op
directly.
> don't do much except calling spi_nor_spimem_setup_op() and
> exec_op() and don't have anything to do with register access
> (except maybe for the write enable). Can't we have a bit more
> sophisticated interface in the core? Something that calls into
> the flash driver to assemble the spi_mem_op automatically? Assuming
Looks liks some ping-pong, passing spi_mem_op directly is straight forward
and can address all manufacturer varieties.
So I prefer keeping these generic methods, it will reduce code duplication.
Thanks,
ta