LinuxLists.cc - [PATCH v1 00/14] mtd: spi-nor: move vendor specific code into vendor modules

[permalink] [raw]

Subject: [PATCH v1 05/14] mtd: spi-nor: xilinx: rename vendor specific functions and defines

Drop the generic spi_nor prefix for all the xilinx functions.

Signed-off-by: Michael Walle <[email protected]>
---
drivers/mtd/spi-nor/xilinx.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/mtd/spi-nor/xilinx.c b/drivers/mtd/spi-nor/xilinx.c
index a865dadc2e5d..3e85530df1e4 100644
--- a/drivers/mtd/spi-nor/xilinx.c
+++ b/drivers/mtd/spi-nor/xilinx.c
@@ -8,9 +8,9 @@

#include "core.h"

-#define SPINOR_OP_XSE 0x50 /* Sector erase */
-#define SPINOR_OP_XPP 0x82 /* Page program */
-#define SPINOR_OP_XRDSR 0xd7 /* Read status register */
+#define XILINX_OP_SE 0x50 /* Sector erase */
+#define XILINX_OP_PP 0x82 /* Page program */
+#define XILINX_OP_RDSR 0xd7 /* Read status register */

#define XSR_PAGESIZE BIT(0) /* Page size in Po2 or Linear */
#define XSR_RDY BIT(7) /* Ready */
@@ -60,20 +60,20 @@ static u32 s3an_convert_addr(struct spi_nor *nor, u32 addr)
}

/**
- * spi_nor_xread_sr() - Read the Status Register on S3AN flashes.
+ * xilinx_read_sr() - Read the Status Register on S3AN flashes.
* @nor: pointer to 'struct spi_nor'.
* @sr: pointer to a DMA-able buffer where the value of the
* Status Register will be written.
*
* Return: 0 on success, -errno otherwise.
*/
-static int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)
+static int xilinx_read_sr(struct spi_nor *nor, u8 *sr)
{
int ret;

if (nor->spimem) {
struct spi_mem_op op =
- SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_XRDSR, 0),
+ SPI_MEM_OP(SPI_MEM_OP_CMD(XILINX_OP_RDSR, 0),
SPI_MEM_OP_NO_ADDR,
SPI_MEM_OP_NO_DUMMY,
SPI_MEM_OP_DATA_IN(1, sr, 0));
@@ -82,7 +82,7 @@ static int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)

ret = spi_mem_exec_op(nor->spimem, &op);
} else {
- ret = spi_nor_controller_ops_read_reg(nor, SPINOR_OP_XRDSR, sr,
+ ret = spi_nor_controller_ops_read_reg(nor, XILINX_OP_RDSR, sr,
1);
}

@@ -99,11 +99,11 @@ static int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)
*
* Return: 1 if ready, 0 if not ready, -errno on errors.
*/
-static int spi_nor_xsr_ready(struct spi_nor *nor)
+static int xilinx_sr_ready(struct spi_nor *nor)
{
int ret;

- ret = spi_nor_xread_sr(nor, nor->bouncebuf);
+ ret = xilinx_read_sr(nor, nor->bouncebuf);
if (ret)
return ret;

@@ -116,12 +116,12 @@ static int xilinx_nor_setup(struct spi_nor *nor,
u32 page_size;
int ret;

- ret = spi_nor_xread_sr(nor, nor->bouncebuf);
+ ret = xilinx_read_sr(nor, nor->bouncebuf);
if (ret)
return ret;

- nor->erase_opcode = SPINOR_OP_XSE;
- nor->program_opcode = SPINOR_OP_XPP;
+ nor->erase_opcode = XILINX_OP_SE;
+ nor->program_opcode = XILINX_OP_PP;
nor->read_opcode = SPINOR_OP_READ;
nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;

@@ -155,7 +155,7 @@ static int xilinx_nor_setup(struct spi_nor *nor,
static void xilinx_late_init(struct spi_nor *nor)
{
nor->params->setup = xilinx_nor_setup;
- nor->params->ready = spi_nor_xsr_ready;
+ nor->params->ready = xilinx_sr_ready;
}

static const struct spi_nor_fixups xilinx_fixups = {
--
2.30.2

2022-02-03 18:49:44

[permalink] [raw]

Subject: [PATCH v1 13/14] mtd: spi-nor: spansion: convert USE_CLSR to a manufacturer flag

Now that all functions using that flag are local to the spanion module,
we can convert the flag to a manufacturer one.

Signed-off-by: Michael Walle <[email protected]>
---
drivers/mtd/spi-nor/core.c | 3 --
drivers/mtd/spi-nor/core.h | 3 --
drivers/mtd/spi-nor/spansion.c | 54 +++++++++++++++++++++-------------
3 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 5b00dfab77a6..2d5517b3db96 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2448,9 +2448,6 @@ static void spi_nor_init_flags(struct spi_nor *nor)

if (flags & NO_CHIP_ERASE)
nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
-
- if (flags & USE_CLSR)
- nor->flags |= SNOR_F_USE_CLSR;
}

/**
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index a02bf54289fb..2130a96e2044 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -14,7 +14,6 @@
enum spi_nor_option_flags {
SNOR_F_HAS_SR_TB = BIT(1),
SNOR_F_NO_OP_CHIP_ERASE = BIT(2),
- SNOR_F_USE_CLSR = BIT(4),
SNOR_F_BROKEN_RESET = BIT(5),
SNOR_F_4B_OPCODES = BIT(6),
SNOR_F_HAS_4BAIT = BIT(7),
@@ -347,7 +346,6 @@ struct spi_nor_fixups {
* SPI_NOR_NO_ERASE: no erase command needed.
* NO_CHIP_ERASE: chip does not support chip erase.
* SPI_NOR_NO_FR: can't do fastread.
- * USE_CLSR: use CLSR command.
*
* @no_sfdp_flags: flags that indicate support that can be discovered via SFDP.
* Used when SFDP tables are not defined in the flash. These
@@ -398,7 +396,6 @@ struct flash_info {
#define SPI_NOR_NO_ERASE BIT(6)
#define NO_CHIP_ERASE BIT(7)
#define SPI_NOR_NO_FR BIT(8)
-#define USE_CLSR BIT(9)

u8 no_sfdp_flags;
#define SPI_NOR_SKIP_SFDP BIT(0)
diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index 4756fb88eab2..c31ea11f71f2 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -8,6 +8,8 @@

#include "core.h"

+#define USE_CLSR BIT(0)
+
#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 */
@@ -212,43 +214,53 @@ static const struct flash_info spansion_parts[] = {
{ "s25sl064p", INFO(0x010216, 0x4d00, 64 * 1024, 128)
NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
{ "s25fl128s0", INFO6(0x012018, 0x4d0080, 256 * 1024, 64)
- FLAGS(USE_CLSR)
- NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+ NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
+ MFR_FLAGS(USE_CLSR)
+ },
{ "s25fl128s1", INFO6(0x012018, 0x4d0180, 64 * 1024, 256)
- FLAGS(USE_CLSR)
- NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+ NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
+ MFR_FLAGS(USE_CLSR)
+ },
{ "s25fl256s0", INFO6(0x010219, 0x4d0080, 256 * 1024, 128)
- FLAGS(USE_CLSR)
NO_SFDP_FLAGS(SPI_NOR_SKIP_SFDP | SPI_NOR_DUAL_READ |
- SPI_NOR_QUAD_READ) },
+ SPI_NOR_QUAD_READ)
+ MFR_FLAGS(USE_CLSR)
+ },
{ "s25fl256s1", INFO6(0x010219, 0x4d0180, 64 * 1024, 512)
- FLAGS(USE_CLSR)
- NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+ NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
+ MFR_FLAGS(USE_CLSR)
+ },
{ "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256)
- FLAGS(SPI_NOR_HAS_LOCK | USE_CLSR)
- NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+ FLAGS(SPI_NOR_HAS_LOCK)
+ NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
+ MFR_FLAGS(USE_CLSR)
+ },
{ "s25fs128s1", INFO6(0x012018, 0x4d0181, 64 * 1024, 256)
- FLAGS(USE_CLSR)
NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
+ MFR_FLAGS(USE_CLSR)
.fixups = &s25fs_s_fixups, },
{ "s25fs256s0", INFO6(0x010219, 0x4d0081, 256 * 1024, 128)
- FLAGS(USE_CLSR)
- NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+ NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
+ MFR_FLAGS(USE_CLSR)
+ },
{ "s25fs256s1", INFO6(0x010219, 0x4d0181, 64 * 1024, 512)
- FLAGS(USE_CLSR)
- NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+ NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
+ MFR_FLAGS(USE_CLSR)
+ },
{ "s25fs512s", INFO6(0x010220, 0x4d0081, 256 * 1024, 256)
- FLAGS(USE_CLSR)
NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
+ MFR_FLAGS(USE_CLSR)
.fixups = &s25fs_s_fixups, },
{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024, 64) },
{ "s25sl12801", INFO(0x012018, 0x0301, 64 * 1024, 256) },
{ "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024, 64)
- FLAGS(USE_CLSR)
- NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+ NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
+ MFR_FLAGS(USE_CLSR)
+ },
{ "s25fl129p1", INFO(0x012018, 0x4d01, 64 * 1024, 256)
- FLAGS(USE_CLSR)
- NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+ NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
+ MFR_FLAGS(USE_CLSR)
+ },
{ "s25sl004a", INFO(0x010212, 0, 64 * 1024, 8) },
{ "s25sl008a", INFO(0x010213, 0, 64 * 1024, 16) },
{ "s25sl016a", INFO(0x010214, 0, 64 * 1024, 32) },
@@ -370,7 +382,7 @@ static void spansion_late_init(struct spi_nor *nor)
nor->mtd.erasesize = nor->info->sector_size;
}

- if (nor->flags & SNOR_F_USE_CLSR)
+ if (nor->info->mfr_flags & USE_CLSR)
nor->params->ready = spi_nor_sr_ready_and_clear;
}

--
2.30.2

2022-02-03 21:26:27

[permalink] [raw]

Subject: [PATCH v1 04/14] mtd: spi-nor: move all xilinx specifics into xilinx.c

Mechanically move all the xilinx functions to its own module.

Then register the new flash specific ready() function.

Signed-off-by: Michael Walle <[email protected]>
---
drivers/mtd/spi-nor/core.c | 64 +------------------------------
drivers/mtd/spi-nor/core.h | 18 ---------
drivers/mtd/spi-nor/xilinx.c | 73 ++++++++++++++++++++++++++++++++++++
include/linux/mtd/spi-nor.h | 9 -----
4 files changed, 74 insertions(+), 90 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index c181f2680df2..fdc8ef623254 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -598,57 +598,6 @@ int spi_nor_write_ear(struct spi_nor *nor, u8 ear)
return ret;
}

-/**
- * spi_nor_xread_sr() - Read the Status Register on S3AN flashes.
- * @nor: pointer to 'struct spi_nor'.
- * @sr: pointer to a DMA-able buffer where the value of the
- * Status Register will be written.
- *
- * Return: 0 on success, -errno otherwise.
- */
-int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)
-{
- int ret;
-
- if (nor->spimem) {
- struct spi_mem_op op =
- SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_XRDSR, 0),
- SPI_MEM_OP_NO_ADDR,
- SPI_MEM_OP_NO_DUMMY,
- SPI_MEM_OP_DATA_IN(1, sr, 0));
-
- spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
-
- ret = spi_mem_exec_op(nor->spimem, &op);
- } else {
- ret = spi_nor_controller_ops_read_reg(nor, SPINOR_OP_XRDSR, sr,
- 1);
- }
-
- if (ret)
- dev_dbg(nor->dev, "error %d reading XRDSR\n", ret);
-
- return ret;
-}
-
-/**
- * spi_nor_xsr_ready() - Query the Status Register of the S3AN flash to see if
- * the flash is ready for new commands.
- * @nor: pointer to 'struct spi_nor'.
- *
- * Return: 1 if ready, 0 if not ready, -errno on errors.
- */
-static int spi_nor_xsr_ready(struct spi_nor *nor)
-{
- int ret;
-
- ret = spi_nor_xread_sr(nor, nor->bouncebuf);
- if (ret)
- return ret;
-
- return !!(nor->bouncebuf[0] & XSR_RDY);
-}
-
/**
* spi_nor_clear_sr() - Clear the Status Register.
* @nor: pointer to 'struct spi_nor'.
@@ -798,10 +747,7 @@ static int spi_nor_ready(struct spi_nor *nor)
if (nor->params->ready)
return nor->params->ready(nor);

- if (nor->flags & SNOR_F_READY_XSR_RDY)
- sr = spi_nor_xsr_ready(nor);
- else
- sr = spi_nor_sr_ready(nor);
+ sr = spi_nor_sr_ready(nor);
if (sr < 0)
return sr;
fsr = nor->flags & SNOR_F_USE_FSR ? spi_nor_fsr_ready(nor) : 1;
@@ -2677,14 +2623,6 @@ static void spi_nor_init_flags(struct spi_nor *nor)

if (flags & USE_FSR)
nor->flags |= SNOR_F_USE_FSR;
-
- /*
- * Make sure the XSR_RDY flag is set before calling
- * spi_nor_wait_till_ready(). Xilinx S3AN share MFR
- * with Atmel SPI NOR.
- */
- if (flags & SPI_NOR_XSR_RDY)
- nor->flags |= SNOR_F_READY_XSR_RDY;
}

/**
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 446218b0e017..fabc01ae9a81 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -15,7 +15,6 @@ enum spi_nor_option_flags {
SNOR_F_USE_FSR = BIT(0),
SNOR_F_HAS_SR_TB = BIT(1),
SNOR_F_NO_OP_CHIP_ERASE = BIT(2),
- SNOR_F_READY_XSR_RDY = BIT(3),
SNOR_F_USE_CLSR = BIT(4),
SNOR_F_BROKEN_RESET = BIT(5),
SNOR_F_4B_OPCODES = BIT(6),
@@ -351,8 +350,6 @@ struct spi_nor_fixups {
* SPI_NOR_NO_FR: can't do fastread.
* USE_CLSR: use CLSR command.
* USE_FSR: use flag status register
- * SPI_NOR_XSR_RDY: S3AN flashes have specific opcode to read the
- * status register.
*
* @no_sfdp_flags: flags that indicate support that can be discovered via SFDP.
* Used when SFDP tables are not defined in the flash. These
@@ -405,7 +402,6 @@ struct flash_info {
#define SPI_NOR_NO_FR BIT(8)
#define USE_CLSR BIT(9)
#define USE_FSR BIT(10)
-#define SPI_NOR_XSR_RDY BIT(11)

u8 no_sfdp_flags;
#define SPI_NOR_SKIP_SFDP BIT(0)
@@ -462,19 +458,6 @@ struct flash_info {
.addr_width = (_addr_width), \
.flags = SPI_NOR_NO_ERASE | SPI_NOR_NO_FR, \

-#define S3AN_INFO(_jedec_id, _n_sectors, _page_size) \
- .id = { \
- ((_jedec_id) >> 16) & 0xff, \
- ((_jedec_id) >> 8) & 0xff, \
- (_jedec_id) & 0xff \
- }, \
- .id_len = 3, \
- .sector_size = (8*_page_size), \
- .n_sectors = (_n_sectors), \
- .page_size = _page_size, \
- .addr_width = 3, \
- .flags = SPI_NOR_NO_FR | SPI_NOR_XSR_RDY,
-
#define OTP_INFO(_len, _n_regions, _base, _offset) \
.otp_org = { \
.len = (_len), \
@@ -564,7 +547,6 @@ int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len);
int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1);
int spi_nor_write_16bit_cr_and_check(struct spi_nor *nor, u8 cr);

-int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr);
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,
diff --git a/drivers/mtd/spi-nor/xilinx.c b/drivers/mtd/spi-nor/xilinx.c
index 580562bc1e45..a865dadc2e5d 100644
--- a/drivers/mtd/spi-nor/xilinx.c
+++ b/drivers/mtd/spi-nor/xilinx.c
@@ -8,6 +8,27 @@

#include "core.h"

+#define SPINOR_OP_XSE 0x50 /* Sector erase */
+#define SPINOR_OP_XPP 0x82 /* Page program */
+#define SPINOR_OP_XRDSR 0xd7 /* Read status register */
+
+#define XSR_PAGESIZE BIT(0) /* Page size in Po2 or Linear */
+#define XSR_RDY BIT(7) /* Ready */
+
+#define S3AN_INFO(_jedec_id, _n_sectors, _page_size) \
+ .id = { \
+ ((_jedec_id) >> 16) & 0xff, \
+ ((_jedec_id) >> 8) & 0xff, \
+ (_jedec_id) & 0xff \
+ }, \
+ .id_len = 3, \
+ .sector_size = (8*_page_size), \
+ .n_sectors = (_n_sectors), \
+ .page_size = _page_size, \
+ .addr_width = 3, \
+ .flags = SPI_NOR_NO_FR
+
+/* Xilinx S3AN share MFR with Atmel SPI NOR */
static const struct flash_info xilinx_parts[] = {
/* Xilinx S3AN Internal Flash */
{ "3S50AN", S3AN_INFO(0x1f2200, 64, 264) },
@@ -38,6 +59,57 @@ static u32 s3an_convert_addr(struct spi_nor *nor, u32 addr)
return page | offset;
}

+/**
+ * spi_nor_xread_sr() - Read the Status Register on S3AN flashes.
+ * @nor: pointer to 'struct spi_nor'.
+ * @sr: pointer to a DMA-able buffer where the value of the
+ * Status Register will be written.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)
+{
+ int ret;
+
+ if (nor->spimem) {
+ struct spi_mem_op op =
+ SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_XRDSR, 0),
+ SPI_MEM_OP_NO_ADDR,
+ SPI_MEM_OP_NO_DUMMY,
+ SPI_MEM_OP_DATA_IN(1, sr, 0));
+
+ spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
+ ret = spi_mem_exec_op(nor->spimem, &op);
+ } else {
+ ret = spi_nor_controller_ops_read_reg(nor, SPINOR_OP_XRDSR, sr,
+ 1);
+ }
+
+ if (ret)
+ dev_dbg(nor->dev, "error %d reading XRDSR\n", ret);
+
+ return ret;
+}
+
+/**
+ * spi_nor_xsr_ready() - Query the Status Register of the S3AN flash to see if
+ * the flash is ready for new commands.
+ * @nor: pointer to 'struct spi_nor'.
+ *
+ * Return: 1 if ready, 0 if not ready, -errno on errors.
+ */
+static int spi_nor_xsr_ready(struct spi_nor *nor)
+{
+ int ret;
+
+ ret = spi_nor_xread_sr(nor, nor->bouncebuf);
+ if (ret)
+ return ret;
+
+ return !!(nor->bouncebuf[0] & XSR_RDY);
+}
+
static int xilinx_nor_setup(struct spi_nor *nor,
const struct spi_nor_hwcaps *hwcaps)
{
@@ -83,6 +155,7 @@ static int xilinx_nor_setup(struct spi_nor *nor,
static void xilinx_late_init(struct spi_nor *nor)
{
nor->params->setup = xilinx_nor_setup;
+ nor->params->ready = spi_nor_xsr_ready;
}

static const struct spi_nor_fixups xilinx_fixups = {
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index fc90fce26e33..b44b05a6f934 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -86,15 +86,6 @@
#define SPINOR_OP_BP 0x02 /* Byte program */
#define SPINOR_OP_AAI_WP 0xad /* Auto address increment word program */

-/* Used for S3AN flashes only */
-#define SPINOR_OP_XSE 0x50 /* Sector erase */
-#define SPINOR_OP_XPP 0x82 /* Page program */
-#define SPINOR_OP_XRDSR 0xd7 /* Read status register */
-
-#define XSR_PAGESIZE BIT(0) /* Page size in Po2 or Linear */
-#define XSR_RDY BIT(7) /* Ready */
-
-
/* Used for Macronix and Winbond flashes. */
#define SPINOR_OP_EN4B 0xb7 /* Enter 4-byte mode */
#define SPINOR_OP_EX4B 0xe9 /* Exit 4-byte mode */
--
2.30.2

2022-02-03 22:54:07

[permalink] [raw]

Subject: [PATCH v1 07/14] mtd: spi-nor: move all micron-st specifics into micron-st.c

The flag status register is only available on micron flashes. Move all
the functions around that into the micron module.

This is almost a mechanical move except for the spi_nor_fsr_ready()
which now also checks the normal status register. Previously, this was
done in spi_nor_ready().

Signed-off-by: Michael Walle <[email protected]>
---
drivers/mtd/spi-nor/core.c | 123 +-----------------------------
drivers/mtd/spi-nor/micron-st.c | 129 ++++++++++++++++++++++++++++++++
include/linux/mtd/spi-nor.h | 8 --
3 files changed, 130 insertions(+), 130 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index fdc8ef623254..e9d9880149d2 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -412,50 +412,6 @@ int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)
return ret;
}

-/**
- * spi_nor_read_fsr() - Read the Flag Status Register.
- * @nor: pointer to 'struct spi_nor'
- * @fsr: pointer to a DMA-able buffer where the value of the
- * Flag Status Register will be written. Should be at least 2
- * bytes.
- *
- * Return: 0 on success, -errno otherwise.
- */
-static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr)
-{
- int ret;
-
- if (nor->spimem) {
- struct spi_mem_op op =
- SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDFSR, 0),
- SPI_MEM_OP_NO_ADDR,
- SPI_MEM_OP_NO_DUMMY,
- SPI_MEM_OP_DATA_IN(1, fsr, 0));
-
- if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR) {
- op.addr.nbytes = nor->params->rdsr_addr_nbytes;
- op.dummy.nbytes = nor->params->rdsr_dummy;
- /*
- * We don't want to read only one byte in DTR mode. So,
- * read 2 and then discard the second byte.
- */
- op.data.nbytes = 2;
- }
-
- spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
-
- ret = spi_mem_exec_op(nor->spimem, &op);
- } else {
- ret = spi_nor_controller_ops_read_reg(nor, SPINOR_OP_RDFSR, fsr,
- 1);
- }
-
- if (ret)
- dev_dbg(nor->dev, "error %d reading FSR\n", ret);
-
- return ret;
-}
-
/**
* spi_nor_read_cr() - Read the Configuration Register using the
* SPINOR_OP_RDCR (35h) command.
@@ -664,75 +620,6 @@ int spi_nor_sr_ready(struct spi_nor *nor)
return !(nor->bouncebuf[0] & SR_WIP);
}

-/**
- * spi_nor_clear_fsr() - Clear the Flag Status Register.
- * @nor: pointer to 'struct spi_nor'.
- */
-static void spi_nor_clear_fsr(struct spi_nor *nor)
-{
- int ret;
-
- if (nor->spimem) {
- struct spi_mem_op op =
- SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLFSR, 0),
- SPI_MEM_OP_NO_ADDR,
- SPI_MEM_OP_NO_DUMMY,
- SPI_MEM_OP_NO_DATA);
-
- spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
-
- ret = spi_mem_exec_op(nor->spimem, &op);
- } else {
- ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLFSR,
- NULL, 0);
- }
-
- if (ret)
- dev_dbg(nor->dev, "error %d clearing FSR\n", ret);
-}
-
-/**
- * spi_nor_fsr_ready() - Query the Flag Status Register to see if the flash is
- * ready for new commands.
- * @nor: pointer to 'struct spi_nor'.
- *
- * Return: 1 if ready, 0 if not ready, -errno on errors.
- */
-static int spi_nor_fsr_ready(struct spi_nor *nor)
-{
- int ret = spi_nor_read_fsr(nor, nor->bouncebuf);
-
- if (ret)
- return ret;
-
- if (nor->bouncebuf[0] & (FSR_E_ERR | FSR_P_ERR)) {
- if (nor->bouncebuf[0] & FSR_E_ERR)
- dev_err(nor->dev, "Erase operation failed.\n");
- else
- dev_err(nor->dev, "Program operation failed.\n");
-
- if (nor->bouncebuf[0] & FSR_PT_ERR)
- dev_err(nor->dev,
- "Attempted to modify a protected sector.\n");
-
- spi_nor_clear_fsr(nor);
-
- /*
- * WEL bit remains set to one when an erase or page program
- * error occurs. Issue a Write Disable command to protect
- * against inadvertent writes that can possibly corrupt the
- * contents of the memory.
- */
- ret = spi_nor_write_disable(nor);
- if (ret)
- return ret;
-
- return -EIO;
- }
-
- return !!(nor->bouncebuf[0] & FSR_READY);
-}
-
/**
* spi_nor_ready() - Query the flash to see if it is ready for new commands.
* @nor: pointer to 'struct spi_nor'.
@@ -741,19 +628,11 @@ static int spi_nor_fsr_ready(struct spi_nor *nor)
*/
static int spi_nor_ready(struct spi_nor *nor)
{
- int sr, fsr;
-
/* flashes might override our standard routine */
if (nor->params->ready)
return nor->params->ready(nor);

- sr = spi_nor_sr_ready(nor);
- if (sr < 0)
- return sr;
- fsr = nor->flags & SNOR_F_USE_FSR ? spi_nor_fsr_ready(nor) : 1;
- if (fsr < 0)
- return fsr;
- return sr && fsr;
+ return spi_nor_sr_ready(nor);
}

/**
diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
index bb95b1aabf74..c66580e8aa00 100644
--- a/drivers/mtd/spi-nor/micron-st.c
+++ b/drivers/mtd/spi-nor/micron-st.c
@@ -8,6 +8,8 @@

#include "core.h"

+#define SPINOR_OP_RDFSR 0x70 /* Read flag status register */
+#define SPINOR_OP_CLFSR 0x50 /* Clear flag status register */
#define SPINOR_OP_MT_DTR_RD 0xfd /* Fast Read opcode in DTR mode */
#define SPINOR_OP_MT_RD_ANY_REG 0x85 /* Read volatile register */
#define SPINOR_OP_MT_WR_ANY_REG 0x81 /* Write volatile register */
@@ -17,6 +19,12 @@
#define SPINOR_MT_OCT_DTR 0xe7 /* Enable Octal DTR. */
#define SPINOR_MT_EXSPI 0xff /* Enable Extended SPI (default) */

+/* Flag Status Register bits */
+#define FSR_READY BIT(7) /* Device status, 0 = Busy, 1 = Ready */
+#define FSR_E_ERR BIT(5) /* Erase operation status */
+#define FSR_P_ERR BIT(4) /* Program operation status */
+#define FSR_PT_ERR BIT(1) /* Protection error bit */
+
static int spi_nor_micron_octal_dtr_enable(struct spi_nor *nor, bool enable)
{
struct spi_mem_op op;
@@ -273,12 +281,133 @@ static int st_micron_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
return spi_nor_write_disable(nor);
}

+/**
+ * spi_nor_read_fsr() - Read the Flag Status Register.
+ * @nor: pointer to 'struct spi_nor'
+ * @fsr: pointer to a DMA-able buffer where the value of the
+ * Flag Status Register will be written. Should be at least 2
+ * bytes.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr)
+{
+ int ret;
+
+ if (nor->spimem) {
+ struct spi_mem_op op =
+ SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDFSR, 0),
+ SPI_MEM_OP_NO_ADDR,
+ SPI_MEM_OP_NO_DUMMY,
+ SPI_MEM_OP_DATA_IN(1, fsr, 0));
+
+ if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR) {
+ op.addr.nbytes = nor->params->rdsr_addr_nbytes;
+ op.dummy.nbytes = nor->params->rdsr_dummy;
+ /*
+ * We don't want to read only one byte in DTR mode. So,
+ * read 2 and then discard the second byte.
+ */
+ op.data.nbytes = 2;
+ }
+
+ spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
+ ret = spi_mem_exec_op(nor->spimem, &op);
+ } else {
+ ret = spi_nor_controller_ops_read_reg(nor, SPINOR_OP_RDFSR, fsr,
+ 1);
+ }
+
+ if (ret)
+ dev_dbg(nor->dev, "error %d reading FSR\n", ret);
+
+ return ret;
+}
+
+/**
+ * spi_nor_clear_fsr() - Clear the Flag Status Register.
+ * @nor: pointer to 'struct spi_nor'.
+ */
+static void spi_nor_clear_fsr(struct spi_nor *nor)
+{
+ int ret;
+
+ if (nor->spimem) {
+ struct spi_mem_op op =
+ SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLFSR, 0),
+ SPI_MEM_OP_NO_ADDR,
+ SPI_MEM_OP_NO_DUMMY,
+ SPI_MEM_OP_NO_DATA);
+
+ spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
+ ret = spi_mem_exec_op(nor->spimem, &op);
+ } else {
+ ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLFSR,
+ NULL, 0);
+ }
+
+ if (ret)
+ dev_dbg(nor->dev, "error %d clearing FSR\n", ret);
+}
+
+/**
+ * spi_nor_fsr_ready() - Query the Flag Status Register to see if the flash is
+ * ready for new commands.
+ * @nor: pointer to 'struct spi_nor'.
+ *
+ * Return: 1 if ready, 0 if not ready, -errno on errors.
+ */
+static int spi_nor_fsr_ready(struct spi_nor *nor)
+{
+ int sr_ready, ret;
+
+ sr_ready = spi_nor_sr_ready(nor);
+ if (sr_ready < 0)
+ return sr_ready;
+
+ ret = spi_nor_read_fsr(nor, nor->bouncebuf);
+ if (ret)
+ return ret;
+
+ if (nor->bouncebuf[0] & (FSR_E_ERR | FSR_P_ERR)) {
+ if (nor->bouncebuf[0] & FSR_E_ERR)
+ dev_err(nor->dev, "Erase operation failed.\n");
+ else
+ dev_err(nor->dev, "Program operation failed.\n");
+
+ if (nor->bouncebuf[0] & FSR_PT_ERR)
+ dev_err(nor->dev,
+ "Attempted to modify a protected sector.\n");
+
+ spi_nor_clear_fsr(nor);
+
+ /*
+ * WEL bit remains set to one when an erase or page program
+ * error occurs. Issue a Write Disable command to protect
+ * against inadvertent writes that can possibly corrupt the
+ * contents of the memory.
+ */
+ ret = spi_nor_write_disable(nor);
+ if (ret)
+ return ret;
+
+ return -EIO;
+ }
+
+ return sr_ready && !!(nor->bouncebuf[0] & FSR_READY);
+}
+
static void micron_st_default_init(struct spi_nor *nor)
{
nor->flags |= SNOR_F_HAS_LOCK;
nor->flags &= ~SNOR_F_HAS_16BIT_SR;
nor->params->quad_enable = NULL;
nor->params->set_4byte_addr_mode = st_micron_set_4byte_addr_mode;
+
+ if (nor->flags & SNOR_F_USE_FSR)
+ nor->params->ready = spi_nor_fsr_ready;
}

static const struct spi_nor_fixups micron_st_fixups = {
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index b44b05a6f934..4622251a79ff 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -47,8 +47,6 @@
#define SPINOR_OP_RDID 0x9f /* Read JEDEC ID */
#define SPINOR_OP_RDSFDP 0x5a /* Read SFDP */
#define SPINOR_OP_RDCR 0x35 /* Read configuration register */
-#define SPINOR_OP_RDFSR 0x70 /* Read flag status register */
-#define SPINOR_OP_CLFSR 0x50 /* Clear flag status register */
#define SPINOR_OP_RDEAR 0xc8 /* Read Extended Address Register */
#define SPINOR_OP_WREAR 0xc5 /* Write Extended Address Register */
#define SPINOR_OP_SRSTEN 0x66 /* Software Reset Enable */
@@ -126,12 +124,6 @@
/* Enhanced Volatile Configuration Register bits */
#define EVCR_QUAD_EN_MICRON BIT(7) /* Micron Quad I/O */

-/* Flag Status Register bits */
-#define FSR_READY BIT(7) /* Device status, 0 = Busy, 1 = Ready */
-#define FSR_E_ERR BIT(5) /* Erase operation status */
-#define FSR_P_ERR BIT(4) /* Program operation status */
-#define FSR_PT_ERR BIT(1) /* Protection error bit */
-
/* Status Register 2 bits. */
#define SR2_QUAD_EN_BIT1 BIT(1)
#define SR2_LB1 BIT(3) /* Security Register Lock Bit 1 */
--
2.30.2

2022-02-04 15:46:49

[permalink] [raw]

Subject: [PATCH v1 06/14] mtd: spi-nor: xilinx: correct the debug message

We are reading the status register, there is no XRDSR register.

Signed-off-by: Michael Walle <[email protected]>
---
drivers/mtd/spi-nor/xilinx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/xilinx.c b/drivers/mtd/spi-nor/xilinx.c
index 3e85530df1e4..9e3ed9609250 100644
--- a/drivers/mtd/spi-nor/xilinx.c
+++ b/drivers/mtd/spi-nor/xilinx.c
@@ -87,7 +87,7 @@ static int xilinx_read_sr(struct spi_nor *nor, u8 *sr)
}

if (ret)
- dev_dbg(nor->dev, "error %d reading XRDSR\n", ret);
+ dev_dbg(nor->dev, "error %d reading SR\n", ret);

return ret;
}
--
2.30.2

2022-02-10 03:27:37

[permalink] [raw]

Subject: Re: [PATCH v1 05/14] mtd: spi-nor: xilinx: rename vendor specific functions and defines

On 2/2/22 16:58, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Drop the generic spi_nor prefix for all the xilinx functions.

mm, no, I would keep the spi_nor prefix because xilinx_sr_ready is too
generic and can conflict with methods from other subsystems.

>
> Signed-off-by: Michael Walle <[email protected]>
> ---
> drivers/mtd/spi-nor/xilinx.c | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/xilinx.c b/drivers/mtd/spi-nor/xilinx.c
> index a865dadc2e5d..3e85530df1e4 100644
> --- a/drivers/mtd/spi-nor/xilinx.c
> +++ b/drivers/mtd/spi-nor/xilinx.c
> @@ -8,9 +8,9 @@
>
> #include "core.h"
>
> -#define SPINOR_OP_XSE 0x50 /* Sector erase */
> -#define SPINOR_OP_XPP 0x82 /* Page program */
> -#define SPINOR_OP_XRDSR 0xd7 /* Read status register */
> +#define XILINX_OP_SE 0x50 /* Sector erase */
> +#define XILINX_OP_PP 0x82 /* Page program */
> +#define XILINX_OP_RDSR 0xd7 /* Read status register */
>
> #define XSR_PAGESIZE BIT(0) /* Page size in Po2 or Linear */
> #define XSR_RDY BIT(7) /* Ready */
> @@ -60,20 +60,20 @@ static u32 s3an_convert_addr(struct spi_nor *nor, u32 addr)
> }
>
> /**
> - * spi_nor_xread_sr() - Read the Status Register on S3AN flashes.
> + * xilinx_read_sr() - Read the Status Register on S3AN flashes.
> * @nor: pointer to 'struct spi_nor'.
> * @sr: pointer to a DMA-able buffer where the value of the
> * Status Register will be written.
> *
> * Return: 0 on success, -errno otherwise.
> */
> -static int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)
> +static int xilinx_read_sr(struct spi_nor *nor, u8 *sr)
> {
> int ret;
>
> if (nor->spimem) {
> struct spi_mem_op op =
> - SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_XRDSR, 0),
> + SPI_MEM_OP(SPI_MEM_OP_CMD(XILINX_OP_RDSR, 0),
> SPI_MEM_OP_NO_ADDR,
> SPI_MEM_OP_NO_DUMMY,
> SPI_MEM_OP_DATA_IN(1, sr, 0));
> @@ -82,7 +82,7 @@ static int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)
>
> ret = spi_mem_exec_op(nor->spimem, &op);
> } else {
> - ret = spi_nor_controller_ops_read_reg(nor, SPINOR_OP_XRDSR, sr,
> + ret = spi_nor_controller_ops_read_reg(nor, XILINX_OP_RDSR, sr,
> 1);
> }
>
> @@ -99,11 +99,11 @@ static int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)
> *
> * Return: 1 if ready, 0 if not ready, -errno on errors.
> */
> -static int spi_nor_xsr_ready(struct spi_nor *nor)
> +static int xilinx_sr_ready(struct spi_nor *nor)
> {
> int ret;
>
> - ret = spi_nor_xread_sr(nor, nor->bouncebuf);
> + ret = xilinx_read_sr(nor, nor->bouncebuf);
> if (ret)
> return ret;
>
> @@ -116,12 +116,12 @@ static int xilinx_nor_setup(struct spi_nor *nor,
> u32 page_size;
> int ret;
>
> - ret = spi_nor_xread_sr(nor, nor->bouncebuf);
> + ret = xilinx_read_sr(nor, nor->bouncebuf);
> if (ret)
> return ret;
>
> - nor->erase_opcode = SPINOR_OP_XSE;
> - nor->program_opcode = SPINOR_OP_XPP;
> + nor->erase_opcode = XILINX_OP_SE;
> + nor->program_opcode = XILINX_OP_PP;
> nor->read_opcode = SPINOR_OP_READ;
> nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
>
> @@ -155,7 +155,7 @@ static int xilinx_nor_setup(struct spi_nor *nor,
> static void xilinx_late_init(struct spi_nor *nor)
> {
> nor->params->setup = xilinx_nor_setup;
> - nor->params->ready = spi_nor_xsr_ready;
> + nor->params->ready = xilinx_sr_ready;
> }
>
> static const struct spi_nor_fixups xilinx_fixups = {
> --
> 2.30.2
>

2022-02-10 03:31:11

[permalink] [raw]

Subject: Re: [PATCH v1 06/14] mtd: spi-nor: xilinx: correct the debug message

On 2/2/22 16:58, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> We are reading the status register, there is no XRDSR register.

don't use the commit message as a continuation of the subject. Tell in the
commit message what you're doing and why it is worth taking it.

with that fixed:
Reviewed-by: Tudor Ambarus <[email protected]>
>
> Signed-off-by: Michael Walle <[email protected]>
> ---
> drivers/mtd/spi-nor/xilinx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/xilinx.c b/drivers/mtd/spi-nor/xilinx.c
> index 3e85530df1e4..9e3ed9609250 100644
> --- a/drivers/mtd/spi-nor/xilinx.c
> +++ b/drivers/mtd/spi-nor/xilinx.c
> @@ -87,7 +87,7 @@ static int xilinx_read_sr(struct spi_nor *nor, u8 *sr)
> }
>
> if (ret)
> - dev_dbg(nor->dev, "error %d reading XRDSR\n", ret);
> + dev_dbg(nor->dev, "error %d reading SR\n", ret);
>
> return ret;
> }
> --
> 2.30.2
>

2022-02-10 04:31:38

[permalink] [raw]

Subject: Re: [PATCH v1 04/14] mtd: spi-nor: move all xilinx specifics into xilinx.c

On 2/2/22 16:58, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Mechanically move all the xilinx functions to its own module.
>
> Then register the new flash specific ready() function.
>
> Signed-off-by: Michael Walle <[email protected]>

excellent!
Reviewed-by: Tudor Ambarus <[email protected]>

> ---
> drivers/mtd/spi-nor/core.c | 64 +------------------------------
> drivers/mtd/spi-nor/core.h | 18 ---------
> drivers/mtd/spi-nor/xilinx.c | 73 ++++++++++++++++++++++++++++++++++++
> include/linux/mtd/spi-nor.h | 9 -----
> 4 files changed, 74 insertions(+), 90 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index c181f2680df2..fdc8ef623254 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -598,57 +598,6 @@ int spi_nor_write_ear(struct spi_nor *nor, u8 ear)
> return ret;
> }
>
> -/**
> - * spi_nor_xread_sr() - Read the Status Register on S3AN flashes.
> - * @nor: pointer to 'struct spi_nor'.
> - * @sr: pointer to a DMA-able buffer where the value of the
> - * Status Register will be written.
> - *
> - * Return: 0 on success, -errno otherwise.
> - */
> -int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)
> -{
> - int ret;
> -
> - if (nor->spimem) {
> - struct spi_mem_op op =
> - SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_XRDSR, 0),
> - SPI_MEM_OP_NO_ADDR,
> - SPI_MEM_OP_NO_DUMMY,
> - SPI_MEM_OP_DATA_IN(1, sr, 0));
> -
> - spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> -
> - ret = spi_mem_exec_op(nor->spimem, &op);
> - } else {
> - ret = spi_nor_controller_ops_read_reg(nor, SPINOR_OP_XRDSR, sr,
> - 1);
> - }
> -
> - if (ret)
> - dev_dbg(nor->dev, "error %d reading XRDSR\n", ret);
> -
> - return ret;
> -}
> -
> -/**
> - * spi_nor_xsr_ready() - Query the Status Register of the S3AN flash to see if
> - * the flash is ready for new commands.
> - * @nor: pointer to 'struct spi_nor'.
> - *
> - * Return: 1 if ready, 0 if not ready, -errno on errors.
> - */
> -static int spi_nor_xsr_ready(struct spi_nor *nor)
> -{
> - int ret;
> -
> - ret = spi_nor_xread_sr(nor, nor->bouncebuf);
> - if (ret)
> - return ret;
> -
> - return !!(nor->bouncebuf[0] & XSR_RDY);
> -}
> -
> /**
> * spi_nor_clear_sr() - Clear the Status Register.
> * @nor: pointer to 'struct spi_nor'.
> @@ -798,10 +747,7 @@ static int spi_nor_ready(struct spi_nor *nor)
> if (nor->params->ready)
> return nor->params->ready(nor);
>
> - if (nor->flags & SNOR_F_READY_XSR_RDY)
> - sr = spi_nor_xsr_ready(nor);
> - else
> - sr = spi_nor_sr_ready(nor);
> + sr = spi_nor_sr_ready(nor);
> if (sr < 0)
> return sr;
> fsr = nor->flags & SNOR_F_USE_FSR ? spi_nor_fsr_ready(nor) : 1;
> @@ -2677,14 +2623,6 @@ static void spi_nor_init_flags(struct spi_nor *nor)
>
> if (flags & USE_FSR)
> nor->flags |= SNOR_F_USE_FSR;
> -
> - /*
> - * Make sure the XSR_RDY flag is set before calling
> - * spi_nor_wait_till_ready(). Xilinx S3AN share MFR
> - * with Atmel SPI NOR.
> - */
> - if (flags & SPI_NOR_XSR_RDY)
> - nor->flags |= SNOR_F_READY_XSR_RDY;
> }
>
> /**
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 446218b0e017..fabc01ae9a81 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -15,7 +15,6 @@ enum spi_nor_option_flags {
> SNOR_F_USE_FSR = BIT(0),
> SNOR_F_HAS_SR_TB = BIT(1),
> SNOR_F_NO_OP_CHIP_ERASE = BIT(2),
> - SNOR_F_READY_XSR_RDY = BIT(3),
> SNOR_F_USE_CLSR = BIT(4),
> SNOR_F_BROKEN_RESET = BIT(5),
> SNOR_F_4B_OPCODES = BIT(6),
> @@ -351,8 +350,6 @@ struct spi_nor_fixups {
> * SPI_NOR_NO_FR: can't do fastread.
> * USE_CLSR: use CLSR command.
> * USE_FSR: use flag status register
> - * SPI_NOR_XSR_RDY: S3AN flashes have specific opcode to read the
> - * status register.
> *
> * @no_sfdp_flags: flags that indicate support that can be discovered via SFDP.
> * Used when SFDP tables are not defined in the flash. These
> @@ -405,7 +402,6 @@ struct flash_info {
> #define SPI_NOR_NO_FR BIT(8)
> #define USE_CLSR BIT(9)
> #define USE_FSR BIT(10)
> -#define SPI_NOR_XSR_RDY BIT(11)
>
> u8 no_sfdp_flags;
> #define SPI_NOR_SKIP_SFDP BIT(0)
> @@ -462,19 +458,6 @@ struct flash_info {
> .addr_width = (_addr_width), \
> .flags = SPI_NOR_NO_ERASE | SPI_NOR_NO_FR, \
>
> -#define S3AN_INFO(_jedec_id, _n_sectors, _page_size) \
> - .id = { \
> - ((_jedec_id) >> 16) & 0xff, \
> - ((_jedec_id) >> 8) & 0xff, \
> - (_jedec_id) & 0xff \
> - }, \
> - .id_len = 3, \
> - .sector_size = (8*_page_size), \
> - .n_sectors = (_n_sectors), \
> - .page_size = _page_size, \
> - .addr_width = 3, \
> - .flags = SPI_NOR_NO_FR | SPI_NOR_XSR_RDY,
> -
> #define OTP_INFO(_len, _n_regions, _base, _offset) \
> .otp_org = { \
> .len = (_len), \
> @@ -564,7 +547,6 @@ int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len);
> int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1);
> int spi_nor_write_16bit_cr_and_check(struct spi_nor *nor, u8 cr);
>
> -int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr);
> 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,
> diff --git a/drivers/mtd/spi-nor/xilinx.c b/drivers/mtd/spi-nor/xilinx.c
> index 580562bc1e45..a865dadc2e5d 100644
> --- a/drivers/mtd/spi-nor/xilinx.c
> +++ b/drivers/mtd/spi-nor/xilinx.c
> @@ -8,6 +8,27 @@
>
> #include "core.h"
>
> +#define SPINOR_OP_XSE 0x50 /* Sector erase */
> +#define SPINOR_OP_XPP 0x82 /* Page program */
> +#define SPINOR_OP_XRDSR 0xd7 /* Read status register */
> +
> +#define XSR_PAGESIZE BIT(0) /* Page size in Po2 or Linear */
> +#define XSR_RDY BIT(7) /* Ready */
> +
> +#define S3AN_INFO(_jedec_id, _n_sectors, _page_size) \
> + .id = { \
> + ((_jedec_id) >> 16) & 0xff, \
> + ((_jedec_id) >> 8) & 0xff, \
> + (_jedec_id) & 0xff \
> + }, \
> + .id_len = 3, \
> + .sector_size = (8*_page_size), \
> + .n_sectors = (_n_sectors), \
> + .page_size = _page_size, \
> + .addr_width = 3, \
> + .flags = SPI_NOR_NO_FR
> +
> +/* Xilinx S3AN share MFR with Atmel SPI NOR */
> static const struct flash_info xilinx_parts[] = {
> /* Xilinx S3AN Internal Flash */
> { "3S50AN", S3AN_INFO(0x1f2200, 64, 264) },
> @@ -38,6 +59,57 @@ static u32 s3an_convert_addr(struct spi_nor *nor, u32 addr)
> return page | offset;
> }
>
> +/**
> + * spi_nor_xread_sr() - Read the Status Register on S3AN flashes.
> + * @nor: pointer to 'struct spi_nor'.
> + * @sr: pointer to a DMA-able buffer where the value of the
> + * Status Register will be written.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)
> +{
> + int ret;
> +
> + if (nor->spimem) {
> + struct spi_mem_op op =
> + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_XRDSR, 0),
> + SPI_MEM_OP_NO_ADDR,
> + SPI_MEM_OP_NO_DUMMY,
> + SPI_MEM_OP_DATA_IN(1, sr, 0));
> +
> + spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> +
> + ret = spi_mem_exec_op(nor->spimem, &op);
> + } else {
> + ret = spi_nor_controller_ops_read_reg(nor, SPINOR_OP_XRDSR, sr,
> + 1);
> + }
> +
> + if (ret)
> + dev_dbg(nor->dev, "error %d reading XRDSR\n", ret);
> +
> + return ret;
> +}
> +
> +/**
> + * spi_nor_xsr_ready() - Query the Status Register of the S3AN flash to see if
> + * the flash is ready for new commands.
> + * @nor: pointer to 'struct spi_nor'.
> + *
> + * Return: 1 if ready, 0 if not ready, -errno on errors.
> + */
> +static int spi_nor_xsr_ready(struct spi_nor *nor)
> +{
> + int ret;
> +
> + ret = spi_nor_xread_sr(nor, nor->bouncebuf);
> + if (ret)
> + return ret;
> +
> + return !!(nor->bouncebuf[0] & XSR_RDY);
> +}
> +
> static int xilinx_nor_setup(struct spi_nor *nor,
> const struct spi_nor_hwcaps *hwcaps)
> {
> @@ -83,6 +155,7 @@ static int xilinx_nor_setup(struct spi_nor *nor,
> static void xilinx_late_init(struct spi_nor *nor)
> {
> nor->params->setup = xilinx_nor_setup;
> + nor->params->ready = spi_nor_xsr_ready;
> }
>
> static const struct spi_nor_fixups xilinx_fixups = {
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index fc90fce26e33..b44b05a6f934 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -86,15 +86,6 @@
> #define SPINOR_OP_BP 0x02 /* Byte program */
> #define SPINOR_OP_AAI_WP 0xad /* Auto address increment word program */
>
> -/* Used for S3AN flashes only */
> -#define SPINOR_OP_XSE 0x50 /* Sector erase */
> -#define SPINOR_OP_XPP 0x82 /* Page program */
> -#define SPINOR_OP_XRDSR 0xd7 /* Read status register */
> -
> -#define XSR_PAGESIZE BIT(0) /* Page size in Po2 or Linear */
> -#define XSR_RDY BIT(7) /* Ready */
> -
> -
> /* Used for Macronix and Winbond flashes. */
> #define SPINOR_OP_EN4B 0xb7 /* Enter 4-byte mode */
> #define SPINOR_OP_EX4B 0xe9 /* Exit 4-byte mode */
> --
> 2.30.2
>

2022-02-10 04:42:09

[permalink] [raw]

Subject: Re: [PATCH v1 13/14] mtd: spi-nor: spansion: convert USE_CLSR to a manufacturer flag

On 2/2/22 16:58, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Now that all functions using that flag are local to the spanion module,
> we can convert the flag to a manufacturer one.
>
> Signed-off-by: Michael Walle <[email protected]>
> ---
> drivers/mtd/spi-nor/core.c | 3 --
> drivers/mtd/spi-nor/core.h | 3 --
> drivers/mtd/spi-nor/spansion.c | 54 +++++++++++++++++++++-------------
> 3 files changed, 33 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 5b00dfab77a6..2d5517b3db96 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2448,9 +2448,6 @@ static void spi_nor_init_flags(struct spi_nor *nor)
>
> if (flags & NO_CHIP_ERASE)
> nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
> -
> - if (flags & USE_CLSR)
> - nor->flags |= SNOR_F_USE_CLSR;
> }
>
> /**
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index a02bf54289fb..2130a96e2044 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -14,7 +14,6 @@
> enum spi_nor_option_flags {
> SNOR_F_HAS_SR_TB = BIT(1),
> SNOR_F_NO_OP_CHIP_ERASE = BIT(2),
> - SNOR_F_USE_CLSR = BIT(4),
> SNOR_F_BROKEN_RESET = BIT(5),
> SNOR_F_4B_OPCODES = BIT(6),
> SNOR_F_HAS_4BAIT = BIT(7),
> @@ -347,7 +346,6 @@ struct spi_nor_fixups {
> * SPI_NOR_NO_ERASE: no erase command needed.
> * NO_CHIP_ERASE: chip does not support chip erase.
> * SPI_NOR_NO_FR: can't do fastread.
> - * USE_CLSR: use CLSR command.
> *
> * @no_sfdp_flags: flags that indicate support that can be discovered via SFDP.
> * Used when SFDP tables are not defined in the flash. These
> @@ -398,7 +396,6 @@ struct flash_info {
> #define SPI_NOR_NO_ERASE BIT(6)
> #define NO_CHIP_ERASE BIT(7)
> #define SPI_NOR_NO_FR BIT(8)
> -#define USE_CLSR BIT(9)
>
> u8 no_sfdp_flags;
> #define SPI_NOR_SKIP_SFDP BIT(0)
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index 4756fb88eab2..c31ea11f71f2 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -8,6 +8,8 @@
>
> #include "core.h"
>
> +#define USE_CLSR BIT(0)

add a description, tell the reader this is a manufacturer specific flag.
excellent work:

Reviewed-by: Tudor Ambarus <[email protected]>

> +
> #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 */
> @@ -212,43 +214,53 @@ static const struct flash_info spansion_parts[] = {
> { "s25sl064p", INFO(0x010216, 0x4d00, 64 * 1024, 128)
> NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> { "s25fl128s0", INFO6(0x012018, 0x4d0080, 256 * 1024, 64)
> - FLAGS(USE_CLSR)
> - NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> + NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> + MFR_FLAGS(USE_CLSR)
> + },
> { "s25fl128s1", INFO6(0x012018, 0x4d0180, 64 * 1024, 256)
> - FLAGS(USE_CLSR)
> - NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> + NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> + MFR_FLAGS(USE_CLSR)
> + },
> { "s25fl256s0", INFO6(0x010219, 0x4d0080, 256 * 1024, 128)
> - FLAGS(USE_CLSR)
> NO_SFDP_FLAGS(SPI_NOR_SKIP_SFDP | SPI_NOR_DUAL_READ |
> - SPI_NOR_QUAD_READ) },
> + SPI_NOR_QUAD_READ)
> + MFR_FLAGS(USE_CLSR)
> + },
> { "s25fl256s1", INFO6(0x010219, 0x4d0180, 64 * 1024, 512)
> - FLAGS(USE_CLSR)
> - NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> + NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> + MFR_FLAGS(USE_CLSR)
> + },
> { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256)
> - FLAGS(SPI_NOR_HAS_LOCK | USE_CLSR)
> - NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> + FLAGS(SPI_NOR_HAS_LOCK)
> + NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> + MFR_FLAGS(USE_CLSR)
> + },
> { "s25fs128s1", INFO6(0x012018, 0x4d0181, 64 * 1024, 256)
> - FLAGS(USE_CLSR)
> NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> + MFR_FLAGS(USE_CLSR)
> .fixups = &s25fs_s_fixups, },
> { "s25fs256s0", INFO6(0x010219, 0x4d0081, 256 * 1024, 128)
> - FLAGS(USE_CLSR)
> - NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> + NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> + MFR_FLAGS(USE_CLSR)
> + },
> { "s25fs256s1", INFO6(0x010219, 0x4d0181, 64 * 1024, 512)
> - FLAGS(USE_CLSR)
> - NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> + NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> + MFR_FLAGS(USE_CLSR)
> + },
> { "s25fs512s", INFO6(0x010220, 0x4d0081, 256 * 1024, 256)
> - FLAGS(USE_CLSR)
> NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> + MFR_FLAGS(USE_CLSR)
> .fixups = &s25fs_s_fixups, },
> { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024, 64) },
> { "s25sl12801", INFO(0x012018, 0x0301, 64 * 1024, 256) },
> { "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024, 64)
> - FLAGS(USE_CLSR)
> - NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> + NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> + MFR_FLAGS(USE_CLSR)
> + },
> { "s25fl129p1", INFO(0x012018, 0x4d01, 64 * 1024, 256)
> - FLAGS(USE_CLSR)
> - NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> + NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> + MFR_FLAGS(USE_CLSR)
> + },
> { "s25sl004a", INFO(0x010212, 0, 64 * 1024, 8) },
> { "s25sl008a", INFO(0x010213, 0, 64 * 1024, 16) },
> { "s25sl016a", INFO(0x010214, 0, 64 * 1024, 32) },
> @@ -370,7 +382,7 @@ static void spansion_late_init(struct spi_nor *nor)
> nor->mtd.erasesize = nor->info->sector_size;
> }
>
> - if (nor->flags & SNOR_F_USE_CLSR)
> + if (nor->info->mfr_flags & USE_CLSR)
> nor->params->ready = spi_nor_sr_ready_and_clear;
> }
>
> --
> 2.30.2
>

2022-02-10 05:41:31

[permalink] [raw]

Subject: Re: [PATCH v1 00/14] mtd: spi-nor: move vendor specific code into vendor modules

On 2/2/22 16:58, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> It turns out that most of the special status register handling is
> specific for a particular vendor. I.e. Xilinx has some different
> opcodes for the status register read, Micron has an additional FSR
> register and Spansion has these flags integrated into the SR.
>
> Create a callback to ready() where a flash chip can register its
> own function. This will let us move all the vendor specific stuff
> out of the core into the vendor modules.
>
> Please note that this is only compile-time tested.
>
> For sake of consistency and better readability of the code flow,
> I also converted the setup() callback to be optional.
>
> Michael Walle (14):
> mtd: spi-nor: export more function to be used in vendor modules
> mtd: spi-nor: slightly refactor the spi_nor_setup()
> mtd: spi-nor: allow a flash to define its own ready() function
> mtd: spi-nor: move all xilinx specifics into xilinx.c
> mtd: spi-nor: xilinx: rename vendor specific functions and defines
> mtd: spi-nor: xilinx: correct the debug message
> mtd: spi-nor: move all micron-st specifics into micron-st.c
> mtd: spi-nor: micron-st: convert USE_FSR to a manufacturer flag
> mtd: spi-nor: micron-st: fix micron_st prefix
> mtd: spi-nor: micron-st: rename vendor specific functions and defines
> mtd: spi-nor: spansion: slightly rework control flow in late_init()
> mtd: spi-nor: move all spansion specifics into spansion.c
> mtd: spi-nor: spansion: convert USE_CLSR to a manufacturer flag
> mtd: spi-nor: renumber flags
>
> drivers/mtd/spi-nor/core.c | 265 ++------------------------------
> drivers/mtd/spi-nor/core.h | 70 ++++-----
> drivers/mtd/spi-nor/micron-st.c | 225 ++++++++++++++++++++++-----
> drivers/mtd/spi-nor/spansion.c | 133 ++++++++++++----
> drivers/mtd/spi-nor/xilinx.c | 79 +++++++++-
> include/linux/mtd/spi-nor.h | 18 ---
> 6 files changed, 417 insertions(+), 373 deletions(-)
>

Good job, Michael! I added R-b to some patches to inform you that they
look fine to me. You don't need to add the tags on the next version, as
I'll be the one that will apply them. I think I have a micron and a spansion
flash somewhere, will try to test your series once v2 is out.

Cheers,
ta

2022-02-10 06:02:42

[permalink] [raw]

Subject: Re: [PATCH v1 07/14] mtd: spi-nor: move all micron-st specifics into micron-st.c

On 2/2/22 16:58, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> The flag status register is only available on micron flashes. Move all
> the functions around that into the micron module.
>
> This is almost a mechanical move except for the spi_nor_fsr_ready()
> which now also checks the normal status register. Previously, this was
> done in spi_nor_ready().
>
> Signed-off-by: Michael Walle <[email protected]>
> ---
> drivers/mtd/spi-nor/core.c | 123 +-----------------------------
> drivers/mtd/spi-nor/micron-st.c | 129 ++++++++++++++++++++++++++++++++
> include/linux/mtd/spi-nor.h | 8 --
> 3 files changed, 130 insertions(+), 130 deletions(-)
>

cut

> diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
> index bb95b1aabf74..c66580e8aa00 100644
> --- a/drivers/mtd/spi-nor/micron-st.c
> +++ b/drivers/mtd/spi-nor/micron-st.c

> static void micron_st_default_init(struct spi_nor *nor)
> {
> nor->flags |= SNOR_F_HAS_LOCK;
> nor->flags &= ~SNOR_F_HAS_16BIT_SR;
> nor->params->quad_enable = NULL;
> nor->params->set_4byte_addr_mode = st_micron_set_4byte_addr_mode;
> +
> + if (nor->flags & SNOR_F_USE_FSR)
> + nor->params->ready = spi_nor_fsr_ready;

I would like to get rid of the default_init hooks. Can't we use late_init for
setting the ready method?

2022-02-10 09:12:21

[permalink] [raw]

Subject: Re: [PATCH v1 05/14] mtd: spi-nor: xilinx: rename vendor specific functions and defines

Am 2022-02-10 04:08, schrieb [email protected]:
> On 2/2/22 16:58, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>> the content is safe
>>
>> Drop the generic spi_nor prefix for all the xilinx functions.
>
> mm, no, I would keep the spi_nor prefix because xilinx_sr_ready is too
> generic and can conflict with methods from other subsystems.

But all the other functions in this file start with xilinx_ ;)

I don't have a strong opinion here, other than it shouldn't
be called spi_nor_read_blaba() because that looks like a
standard spi nor function belonging in core.c

-michael

2022-02-10 10:07:18

[permalink] [raw]

Subject: Re: [PATCH v1 05/14] mtd: spi-nor: xilinx: rename vendor specific functions and defines

On 2/10/22 10:04, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Am 2022-02-10 04:08, schrieb [email protected]:
>> On 2/2/22 16:58, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Drop the generic spi_nor prefix for all the xilinx functions.
>>
>> mm, no, I would keep the spi_nor prefix because xilinx_sr_ready is too
>> generic and can conflict with methods from other subsystems.
>
> But all the other functions in this file start with xilinx_ ;)
>
> I don't have a strong opinion here, other than it shouldn't
> be called spi_nor_read_blaba() because that looks like a
> standard spi nor function belonging in core.c
>

then let's prepend all with spi_nor_xilinx_*()?

2022-02-15 09:08:50

[permalink] [raw]

Subject: Re: [PATCH v1 05/14] mtd: spi-nor: xilinx: rename vendor specific functions and defines

Miquel in To:

On 2/15/22 10:25, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Am 2022-02-10 09:06, schrieb [email protected]:
>> On 2/10/22 10:04, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Am 2022-02-10 04:08, schrieb [email protected]:
>>>> On 2/2/22 16:58, Michael Walle wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>> know
>>>>> the content is safe
>>>>>
>>>>> Drop the generic spi_nor prefix for all the xilinx functions.
>>>>
>>>> mm, no, I would keep the spi_nor prefix because xilinx_sr_ready is
>>>> too
>>>> generic and can conflict with methods from other subsystems.
>>>
>>> But all the other functions in this file start with xilinx_ ;)
>>>
>>> I don't have a strong opinion here, other than it shouldn't
>>> be called spi_nor_read_blaba() because that looks like a
>>> standard spi nor function belonging in core.c
>>>
>>
>> then let's prepend all with spi_nor_xilinx_*()?
>
> I'm still not sure what to do here. Have a look at all the other
> vendor modules in spi-nor. they are all prefixed with the vendor
> name? E.g. there is a sst_write() which is far more likely to
> cause a conflict. So should we rename all these functions? Or
> do we just take our chance that it might have a conflict in
> the future (with an easy fix to rename the function then). TBH
> I doubt there will be a global symbol "xilinx_read_sr()".

I doubt it will not be a conflict.

>
> But I care for consistency, so having some named xilinx_, sst_,
> st_micron_ and some spi_nor_read_xsr sounds and looks awful.

yes, I agree. Take a look on what's happening in NAND. They prepend
the name with vendor_nand_*(). Or in SPI NAND they use flash family
names which should be unique. So how about aligning with NAND and
use vendor_nor_*()?

2022-02-15 12:43:15

[permalink] [raw]

Subject: Re: [PATCH v1 05/14] mtd: spi-nor: xilinx: rename vendor specific functions and defines

Am 2022-02-15 09:52, schrieb [email protected]:
> Miquel in To:
>
> On 2/15/22 10:25, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>> the content is safe
>>
>> Am 2022-02-10 09:06, schrieb [email protected]:
>>> On 2/10/22 10:04, Michael Walle wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>> know
>>>> the content is safe
>>>>
>>>> Am 2022-02-10 04:08, schrieb [email protected]:
>>>>> On 2/2/22 16:58, Michael Walle wrote:
>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>>> know
>>>>>> the content is safe
>>>>>>
>>>>>> Drop the generic spi_nor prefix for all the xilinx functions.
>>>>>
>>>>> mm, no, I would keep the spi_nor prefix because xilinx_sr_ready is
>>>>> too
>>>>> generic and can conflict with methods from other subsystems.
>>>>
>>>> But all the other functions in this file start with xilinx_ ;)
>>>>
>>>> I don't have a strong opinion here, other than it shouldn't
>>>> be called spi_nor_read_blaba() because that looks like a
>>>> standard spi nor function belonging in core.c
>>>>
>>>
>>> then let's prepend all with spi_nor_xilinx_*()?
>>
>> I'm still not sure what to do here. Have a look at all the other
>> vendor modules in spi-nor. they are all prefixed with the vendor
>> name? E.g. there is a sst_write() which is far more likely to
>> cause a conflict. So should we rename all these functions? Or
>> do we just take our chance that it might have a conflict in
>> the future (with an easy fix to rename the function then). TBH
>> I doubt there will be a global symbol "xilinx_read_sr()".
>
> I doubt it will not be a conflict.
>
>>
>> But I care for consistency, so having some named xilinx_, sst_,
>> st_micron_ and some spi_nor_read_xsr sounds and looks awful.
>
> yes, I agree. Take a look on what's happening in NAND. They prepend
> the name with vendor_nand_*(). Or in SPI NAND they use flash family
> names which should be unique. So how about aligning with NAND and
> use vendor_nor_*()?

Sounds good. Regarding the flash family.. take a look at Winbond W25M
which can either be NAND or NOR depending on the size ;)

But the main question was rather whether we rename all the function
names at once or bit by bit. To proceed here with this series, I'd
use the vendor_nor_ prefix for the moved functions (but still keep
the micron_st_ st_micron_ rename patch).

-michael

2022-02-15 14:20:19

[permalink] [raw]

Subject: Re: [PATCH v1 05/14] mtd: spi-nor: xilinx: rename vendor specific functions and defines

Am 2022-02-10 09:06, schrieb [email protected]:
> On 2/10/22 10:04, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>> the content is safe
>>
>> Am 2022-02-10 04:08, schrieb [email protected]:
>>> On 2/2/22 16:58, Michael Walle wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>> know
>>>> the content is safe
>>>>
>>>> Drop the generic spi_nor prefix for all the xilinx functions.
>>>
>>> mm, no, I would keep the spi_nor prefix because xilinx_sr_ready is
>>> too
>>> generic and can conflict with methods from other subsystems.
>>
>> But all the other functions in this file start with xilinx_ ;)
>>
>> I don't have a strong opinion here, other than it shouldn't
>> be called spi_nor_read_blaba() because that looks like a
>> standard spi nor function belonging in core.c
>>
>
> then let's prepend all with spi_nor_xilinx_*()?

I'm still not sure what to do here. Have a look at all the other
vendor modules in spi-nor. they are all prefixed with the vendor
name? E.g. there is a sst_write() which is far more likely to
cause a conflict. So should we rename all these functions? Or
do we just take our chance that it might have a conflict in
the future (with an easy fix to rename the function then). TBH
I doubt there will be a global symbol "xilinx_read_sr()".

But I care for consistency, so having some named xilinx_, sst_,
st_micron_ and some spi_nor_read_xsr sounds and looks awful.

-michael

2022-02-15 19:23:03

[permalink] [raw]

Subject: Re: [PATCH v1 05/14] mtd: spi-nor: xilinx: rename vendor specific functions and defines

On 2/15/22 11:58, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Am 2022-02-15 09:52, schrieb [email protected]:
>> Miquel in To:
>>
>> On 2/15/22 10:25, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Am 2022-02-10 09:06, schrieb [email protected]:
>>>> On 2/10/22 10:04, Michael Walle wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>> know
>>>>> the content is safe
>>>>>
>>>>> Am 2022-02-10 04:08, schrieb [email protected]:
>>>>>> On 2/2/22 16:58, Michael Walle wrote:
>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>>>> know
>>>>>>> the content is safe
>>>>>>>
>>>>>>> Drop the generic spi_nor prefix for all the xilinx functions.
>>>>>>
>>>>>> mm, no, I would keep the spi_nor prefix because xilinx_sr_ready is
>>>>>> too
>>>>>> generic and can conflict with methods from other subsystems.
>>>>>
>>>>> But all the other functions in this file start with xilinx_ ;)
>>>>>
>>>>> I don't have a strong opinion here, other than it shouldn't
>>>>> be called spi_nor_read_blaba() because that looks like a
>>>>> standard spi nor function belonging in core.c
>>>>>
>>>>
>>>> then let's prepend all with spi_nor_xilinx_*()?
>>>
>>> I'm still not sure what to do here. Have a look at all the other
>>> vendor modules in spi-nor. they are all prefixed with the vendor
>>> name? E.g. there is a sst_write() which is far more likely to
>>> cause a conflict. So should we rename all these functions? Or
>>> do we just take our chance that it might have a conflict in
>>> the future (with an easy fix to rename the function then). TBH
>>> I doubt there will be a global symbol "xilinx_read_sr()".
>>
>> I doubt it will not be a conflict.
>>
>>>
>>> But I care for consistency, so having some named xilinx_, sst_,
>>> st_micron_ and some spi_nor_read_xsr sounds and looks awful.
>>
>> yes, I agree. Take a look on what's happening in NAND. They prepend
>> the name with vendor_nand_*(). Or in SPI NAND they use flash family
>> names which should be unique. So how about aligning with NAND and
>> use vendor_nor_*()?
>
> Sounds good. Regarding the flash family.. take a look at Winbond W25M
> which can either be NAND or NOR depending on the size ;)

right, vendor_nor_*() should be just fine

>
> But the main question was rather whether we rename all the function
> names at once or bit by bit. To proceed here with this series, I'd
> use the vendor_nor_ prefix for the moved functions (but still keep
> the micron_st_ st_micron_ rename patch).
>

let's rename them all in a single patch before moving the vendor code
out of the core, and then you can fit with the moved bits in an
elegant way ;)

2022-02-15 19:31:30

[permalink] [raw]

Subject: Re: [PATCH v1 04/14] mtd: spi-nor: move all xilinx specifics into xilinx.c

On 02/02/22 03:58PM, Michael Walle wrote:
> Mechanically move all the xilinx functions to its own module.

If you do some code changes mechanically you should also paste the
semantic patch for future reference.

>
> Then register the new flash specific ready() function.
>
> Signed-off-by: Michael Walle <[email protected]>
> ---
> drivers/mtd/spi-nor/core.c | 64 +------------------------------
> drivers/mtd/spi-nor/core.h | 18 ---------
> drivers/mtd/spi-nor/xilinx.c | 73 ++++++++++++++++++++++++++++++++++++
> include/linux/mtd/spi-nor.h | 9 -----
> 4 files changed, 74 insertions(+), 90 deletions(-)
>
[...]
>
> +/**
> + * spi_nor_xread_sr() - Read the Status Register on S3AN flashes.
> + * @nor: pointer to 'struct spi_nor'.
> + * @sr: pointer to a DMA-able buffer where the value of the
> + * Status Register will be written.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)

Nitpick: rename to xilinx_read_sr()?

> +{
> + int ret;
> +
> + if (nor->spimem) {
> + struct spi_mem_op op =
> + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_XRDSR, 0),
> + SPI_MEM_OP_NO_ADDR,
> + SPI_MEM_OP_NO_DUMMY,
> + SPI_MEM_OP_DATA_IN(1, sr, 0));
> +
> + spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> +
> + ret = spi_mem_exec_op(nor->spimem, &op);
> + } else {
> + ret = spi_nor_controller_ops_read_reg(nor, SPINOR_OP_XRDSR, sr,
> + 1);
> + }
> +
> + if (ret)
> + dev_dbg(nor->dev, "error %d reading XRDSR\n", ret);
> +
> + return ret;
> +}
> +
> +/**
> + * spi_nor_xsr_ready() - Query the Status Register of the S3AN flash to see if
> + * the flash is ready for new commands.
> + * @nor: pointer to 'struct spi_nor'.
> + *
> + * Return: 1 if ready, 0 if not ready, -errno on errors.
> + */
> +static int spi_nor_xsr_ready(struct spi_nor *nor)

Nitpick: rename to xilinx_sr_ready()?

Either way,

Reviewed-by: Pratyush Yadav <[email protected]>

--
Regards,
Pratyush Yadav
Texas Instruments Inc.

2022-02-16 04:16:16

[permalink] [raw]

Subject: Re: [PATCH v1 13/14] mtd: spi-nor: spansion: convert USE_CLSR to a manufacturer flag

On 10/02/22 03:34AM, [email protected] wrote:
> On 2/2/22 16:58, Michael Walle wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > Now that all functions using that flag are local to the spanion module,

s/spanion/spansion/

> > we can convert the flag to a manufacturer one.
> >
> > Signed-off-by: Michael Walle <[email protected]>
> > ---
> > drivers/mtd/spi-nor/core.c | 3 --
> > drivers/mtd/spi-nor/core.h | 3 --
> > drivers/mtd/spi-nor/spansion.c | 54 +++++++++++++++++++++-------------
> > 3 files changed, 33 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index 5b00dfab77a6..2d5517b3db96 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -2448,9 +2448,6 @@ static void spi_nor_init_flags(struct spi_nor *nor)
> >
> > if (flags & NO_CHIP_ERASE)
> > nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
> > -
> > - if (flags & USE_CLSR)
> > - nor->flags |= SNOR_F_USE_CLSR;
> > }
> >
> > /**
> > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> > index a02bf54289fb..2130a96e2044 100644
> > --- a/drivers/mtd/spi-nor/core.h
> > +++ b/drivers/mtd/spi-nor/core.h
> > @@ -14,7 +14,6 @@
> > enum spi_nor_option_flags {
> > SNOR_F_HAS_SR_TB = BIT(1),
> > SNOR_F_NO_OP_CHIP_ERASE = BIT(2),
> > - SNOR_F_USE_CLSR = BIT(4),
> > SNOR_F_BROKEN_RESET = BIT(5),
> > SNOR_F_4B_OPCODES = BIT(6),
> > SNOR_F_HAS_4BAIT = BIT(7),
> > @@ -347,7 +346,6 @@ struct spi_nor_fixups {
> > * SPI_NOR_NO_ERASE: no erase command needed.
> > * NO_CHIP_ERASE: chip does not support chip erase.
> > * SPI_NOR_NO_FR: can't do fastread.
> > - * USE_CLSR: use CLSR command.
> > *
> > * @no_sfdp_flags: flags that indicate support that can be discovered via SFDP.
> > * Used when SFDP tables are not defined in the flash. These
> > @@ -398,7 +396,6 @@ struct flash_info {
> > #define SPI_NOR_NO_ERASE BIT(6)
> > #define NO_CHIP_ERASE BIT(7)
> > #define SPI_NOR_NO_FR BIT(8)
> > -#define USE_CLSR BIT(9)
> >
> > u8 no_sfdp_flags;
> > #define SPI_NOR_SKIP_SFDP BIT(0)
> > diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> > index 4756fb88eab2..c31ea11f71f2 100644
> > --- a/drivers/mtd/spi-nor/spansion.c
> > +++ b/drivers/mtd/spi-nor/spansion.c
> > @@ -8,6 +8,8 @@
> >
> > #include "core.h"
> >
> > +#define USE_CLSR BIT(0)
>
> add a description, tell the reader this is a manufacturer specific flag.

+1

> excellent work:

+1

>
> Reviewed-by: Tudor Ambarus <[email protected]>

Reviewed-by: Pratyush Yadav <[email protected]>

>
> > +
> > #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 */
> > @@ -212,43 +214,53 @@ static const struct flash_info spansion_parts[] = {
> > { "s25sl064p", INFO(0x010216, 0x4d00, 64 * 1024, 128)
> > NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> > { "s25fl128s0", INFO6(0x012018, 0x4d0080, 256 * 1024, 64)
> > - FLAGS(USE_CLSR)
> > - NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> > + NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> > + MFR_FLAGS(USE_CLSR)
> > + },
> > { "s25fl128s1", INFO6(0x012018, 0x4d0180, 64 * 1024, 256)
> > - FLAGS(USE_CLSR)
> > - NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> > + NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> > + MFR_FLAGS(USE_CLSR)
> > + },
> > { "s25fl256s0", INFO6(0x010219, 0x4d0080, 256 * 1024, 128)
> > - FLAGS(USE_CLSR)
> > NO_SFDP_FLAGS(SPI_NOR_SKIP_SFDP | SPI_NOR_DUAL_READ |
> > - SPI_NOR_QUAD_READ) },
> > + SPI_NOR_QUAD_READ)
> > + MFR_FLAGS(USE_CLSR)
> > + },
> > { "s25fl256s1", INFO6(0x010219, 0x4d0180, 64 * 1024, 512)
> > - FLAGS(USE_CLSR)
> > - NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> > + NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> > + MFR_FLAGS(USE_CLSR)
> > + },
> > { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256)
> > - FLAGS(SPI_NOR_HAS_LOCK | USE_CLSR)
> > - NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> > + FLAGS(SPI_NOR_HAS_LOCK)
> > + NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> > + MFR_FLAGS(USE_CLSR)
> > + },
> > { "s25fs128s1", INFO6(0x012018, 0x4d0181, 64 * 1024, 256)
> > - FLAGS(USE_CLSR)
> > NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> > + MFR_FLAGS(USE_CLSR)
> > .fixups = &s25fs_s_fixups, },
> > { "s25fs256s0", INFO6(0x010219, 0x4d0081, 256 * 1024, 128)
> > - FLAGS(USE_CLSR)
> > - NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> > + NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> > + MFR_FLAGS(USE_CLSR)
> > + },
> > { "s25fs256s1", INFO6(0x010219, 0x4d0181, 64 * 1024, 512)
> > - FLAGS(USE_CLSR)
> > - NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> > + NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> > + MFR_FLAGS(USE_CLSR)
> > + },
> > { "s25fs512s", INFO6(0x010220, 0x4d0081, 256 * 1024, 256)
> > - FLAGS(USE_CLSR)
> > NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> > + MFR_FLAGS(USE_CLSR)
> > .fixups = &s25fs_s_fixups, },
> > { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024, 64) },
> > { "s25sl12801", INFO(0x012018, 0x0301, 64 * 1024, 256) },
> > { "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024, 64)
> > - FLAGS(USE_CLSR)
> > - NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> > + NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> > + MFR_FLAGS(USE_CLSR)
> > + },
> > { "s25fl129p1", INFO(0x012018, 0x4d01, 64 * 1024, 256)
> > - FLAGS(USE_CLSR)
> > - NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> > + NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> > + MFR_FLAGS(USE_CLSR)
> > + },
> > { "s25sl004a", INFO(0x010212, 0, 64 * 1024, 8) },
> > { "s25sl008a", INFO(0x010213, 0, 64 * 1024, 16) },
> > { "s25sl016a", INFO(0x010214, 0, 64 * 1024, 32) },
> > @@ -370,7 +382,7 @@ static void spansion_late_init(struct spi_nor *nor)
> > nor->mtd.erasesize = nor->info->sector_size;
> > }
> >
> > - if (nor->flags & SNOR_F_USE_CLSR)
> > + if (nor->info->mfr_flags & USE_CLSR)
> > nor->params->ready = spi_nor_sr_ready_and_clear;
> > }
> >
> > --
> > 2.30.2
> >
>

--
Regards,
Pratyush Yadav
Texas Instruments Inc.

2022-02-16 07:00:06

[permalink] [raw]

Subject: Re: [PATCH v1 06/14] mtd: spi-nor: xilinx: correct the debug message

On 10/02/22 03:11AM, [email protected] wrote:
> On 2/2/22 16:58, Michael Walle wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > We are reading the status register, there is no XRDSR register.
>
> don't use the commit message as a continuation of the subject. Tell in the
> commit message what you're doing and why it is worth taking it.
>
> with that fixed:
> Reviewed-by: Tudor Ambarus <[email protected]>

Reviewed-by: Pratyush Yadav <[email protected]>

--
Regards,
Pratyush Yadav
Texas Instruments Inc.

2022-02-16 07:31:41

[permalink] [raw]

Subject: Re: [PATCH v1 07/14] mtd: spi-nor: move all micron-st specifics into micron-st.c

On 02/02/22 03:58PM, Michael Walle wrote:
> The flag status register is only available on micron flashes. Move all
> the functions around that into the micron module.
>
> This is almost a mechanical move except for the spi_nor_fsr_ready()
> which now also checks the normal status register. Previously, this was
> done in spi_nor_ready().
>
> Signed-off-by: Michael Walle <[email protected]>
> ---
> drivers/mtd/spi-nor/core.c | 123 +-----------------------------
> drivers/mtd/spi-nor/micron-st.c | 129 ++++++++++++++++++++++++++++++++
> include/linux/mtd/spi-nor.h | 8 --
> 3 files changed, 130 insertions(+), 130 deletions(-)
>
[...]
> diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
> index bb95b1aabf74..c66580e8aa00 100644
> --- a/drivers/mtd/spi-nor/micron-st.c
> +++ b/drivers/mtd/spi-nor/micron-st.c
> @@ -8,6 +8,8 @@
>
> #include "core.h"
>
> +#define SPINOR_OP_RDFSR 0x70 /* Read flag status register */
> +#define SPINOR_OP_CLFSR 0x50 /* Clear flag status register */
> #define SPINOR_OP_MT_DTR_RD 0xfd /* Fast Read opcode in DTR mode */
> #define SPINOR_OP_MT_RD_ANY_REG 0x85 /* Read volatile register */
> #define SPINOR_OP_MT_WR_ANY_REG 0x81 /* Write volatile register */
> @@ -17,6 +19,12 @@
> #define SPINOR_MT_OCT_DTR 0xe7 /* Enable Octal DTR. */
> #define SPINOR_MT_EXSPI 0xff /* Enable Extended SPI (default) */
>
> +/* Flag Status Register bits */
> +#define FSR_READY BIT(7) /* Device status, 0 = Busy, 1 = Ready */
> +#define FSR_E_ERR BIT(5) /* Erase operation status */
> +#define FSR_P_ERR BIT(4) /* Program operation status */
> +#define FSR_PT_ERR BIT(1) /* Protection error bit */
> +
> static int spi_nor_micron_octal_dtr_enable(struct spi_nor *nor, bool enable)
> {
> struct spi_mem_op op;
> @@ -273,12 +281,133 @@ static int st_micron_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
> return spi_nor_write_disable(nor);
> }
>
> +/**
> + * spi_nor_read_fsr() - Read the Flag Status Register.
> + * @nor: pointer to 'struct spi_nor'
> + * @fsr: pointer to a DMA-able buffer where the value of the
> + * Flag Status Register will be written. Should be at least 2
> + * bytes.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr)
> +{
> + int ret;
> +
> + if (nor->spimem) {
> + struct spi_mem_op op =
> + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDFSR, 0),
> + SPI_MEM_OP_NO_ADDR,
> + SPI_MEM_OP_NO_DUMMY,
> + SPI_MEM_OP_DATA_IN(1, fsr, 0));
> +
> + if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR) {
> + op.addr.nbytes = nor->params->rdsr_addr_nbytes;
> + op.dummy.nbytes = nor->params->rdsr_dummy;
> + /*
> + * We don't want to read only one byte in DTR mode. So,
> + * read 2 and then discard the second byte.
> + */
> + op.data.nbytes = 2;
> + }
> +
> + spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> +
> + ret = spi_mem_exec_op(nor->spimem, &op);
> + } else {
> + ret = spi_nor_controller_ops_read_reg(nor, SPINOR_OP_RDFSR, fsr,
> + 1);
> + }
> +
> + if (ret)
> + dev_dbg(nor->dev, "error %d reading FSR\n", ret);
> +
> + return ret;
> +}
> +
> +/**
> + * spi_nor_clear_fsr() - Clear the Flag Status Register.
> + * @nor: pointer to 'struct spi_nor'.
> + */
> +static void spi_nor_clear_fsr(struct spi_nor *nor)
> +{
> + int ret;
> +
> + if (nor->spimem) {
> + struct spi_mem_op op =
> + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLFSR, 0),
> + SPI_MEM_OP_NO_ADDR,
> + SPI_MEM_OP_NO_DUMMY,
> + SPI_MEM_OP_NO_DATA);
> +
> + spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> +
> + ret = spi_mem_exec_op(nor->spimem, &op);
> + } else {
> + ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLFSR,
> + NULL, 0);
> + }
> +
> + if (ret)
> + dev_dbg(nor->dev, "error %d clearing FSR\n", ret);
> +}
> +
> +/**
> + * spi_nor_fsr_ready() - Query the Flag Status Register to see if the flash is
> + * ready for new commands.
> + * @nor: pointer to 'struct spi_nor'.
> + *
> + * Return: 1 if ready, 0 if not ready, -errno on errors.
> + */
> +static int spi_nor_fsr_ready(struct spi_nor *nor)

Nitpick: At this point this function is not just spi_nor_fsr_ready(). I
think it should be renamed to something more accurate like
micron_st_ready() (with whatever prefix scheme that was decided upon).

Looks good otherwise.

Reviewed-by: Pratyush Yadav <[email protected]>

--
Regards,
Pratyush Yadav
Texas Instruments Inc.

2022-02-17 16:06:58