It's time for some spring cleaning. Remove the oddball xilinx
flashes with non-power-of-2 page sizes.
Remove the .setup() callback, only the default callback is ever
used and it is unlikely there is need for a custom setup.
Finally, the last patch is a proposal how to deprecate flashes,
which are just detected by their id. We cannot really find out if
there are boards out there which are using a particular flash. Thus,
as a first step, we can print a warning during kernel startup. As a
second step we might introduce a kernel config option to actually
disable the flashes which has the deprecated flag.
Michael Walle (6):
mtd: spi-nor: Remove support for Xilinx S3AN flashes
mtd: spi-nor: get rid of non-power-of-2 page size handling
mtd: spi-nor: get rid of SPI_NOR_NO_FR
mtd: spi-nor: remove .setup() callback
mtd: spi-nor: simplify spi_nor_get_flash_info()
mtd: spi-nor: introduce support for displaying deprecation message
drivers/mtd/spi-nor/Makefile | 1 -
drivers/mtd/spi-nor/core.c | 202 +++++++++++++++------------------
drivers/mtd/spi-nor/core.h | 9 +-
drivers/mtd/spi-nor/everspin.c | 19 +++-
drivers/mtd/spi-nor/xilinx.c | 169 ---------------------------
5 files changed, 110 insertions(+), 290 deletions(-)
delete mode 100644 drivers/mtd/spi-nor/xilinx.c
--
2.39.2
These flashes are kind of an oddball for the very old Xilinx Spartan 3
FPGAs to store their bitstream. More importantly, they reuse the Atmel
JEDEC manufacturer ID and in fact the at45db081d already blocks the use
of the 3S700AN flash chip. It's time to sunset support for these
flashes.
Signed-off-by: Michael Walle <[email protected]>
---
drivers/mtd/spi-nor/Makefile | 1 -
drivers/mtd/spi-nor/core.c | 1 -
drivers/mtd/spi-nor/core.h | 1 -
drivers/mtd/spi-nor/xilinx.c | 169 -----------------------------------
4 files changed, 172 deletions(-)
delete mode 100644 drivers/mtd/spi-nor/xilinx.c
diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
index 5e68468b72fc..5dd9c35f6b6f 100644
--- a/drivers/mtd/spi-nor/Makefile
+++ b/drivers/mtd/spi-nor/Makefile
@@ -13,7 +13,6 @@ spi-nor-objs += micron-st.o
spi-nor-objs += spansion.o
spi-nor-objs += sst.o
spi-nor-objs += winbond.o
-spi-nor-objs += xilinx.o
spi-nor-objs += xmc.o
spi-nor-$(CONFIG_DEBUG_FS) += debugfs.o
obj-$(CONFIG_MTD_SPI_NOR) += spi-nor.o
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 3e1f1913536b..cbe5f92eb0af 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1986,7 +1986,6 @@ static const struct spi_nor_manufacturer *manufacturers[] = {
&spi_nor_spansion,
&spi_nor_sst,
&spi_nor_winbond,
- &spi_nor_xilinx,
&spi_nor_xmc,
};
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 442786685515..072c69b0d06c 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -603,7 +603,6 @@ extern const struct spi_nor_manufacturer spi_nor_st;
extern const struct spi_nor_manufacturer spi_nor_spansion;
extern const struct spi_nor_manufacturer spi_nor_sst;
extern const struct spi_nor_manufacturer spi_nor_winbond;
-extern const struct spi_nor_manufacturer spi_nor_xilinx;
extern const struct spi_nor_manufacturer spi_nor_xmc;
extern const struct attribute_group *spi_nor_sysfs_groups[];
diff --git a/drivers/mtd/spi-nor/xilinx.c b/drivers/mtd/spi-nor/xilinx.c
deleted file mode 100644
index f99118c691b0..000000000000
--- a/drivers/mtd/spi-nor/xilinx.c
+++ /dev/null
@@ -1,169 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Copyright (C) 2005, Intec Automation Inc.
- * Copyright (C) 2014, Freescale Semiconductor, Inc.
- */
-
-#include <linux/mtd/spi-nor.h>
-
-#include "core.h"
-
-#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 */
-
-#define XILINX_RDSR_OP(buf) \
- 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, buf, 0))
-
-#define S3AN_FLASH(_id, _name, _n_sectors, _page_size) \
- .id = _id, \
- .name = _name, \
- .size = 8 * (_page_size) * (_n_sectors), \
- .sector_size = (8 * (_page_size)), \
- .page_size = (_page_size), \
- .flags = SPI_NOR_NO_FR
-
-/* Xilinx S3AN share MFR with Atmel SPI NOR */
-static const struct flash_info xilinx_nor_parts[] = {
- /* Xilinx S3AN Internal Flash */
- { S3AN_FLASH(SNOR_ID(0x1f, 0x22, 0x00), "3S50AN", 64, 264) },
- { S3AN_FLASH(SNOR_ID(0x1f, 0x24, 0x00), "3S200AN", 256, 264) },
- { S3AN_FLASH(SNOR_ID(0x1f, 0x24, 0x00), "3S400AN", 256, 264) },
- { S3AN_FLASH(SNOR_ID(0x1f, 0x25, 0x00), "3S700AN", 512, 264) },
- { S3AN_FLASH(SNOR_ID(0x1f, 0x26, 0x00), "3S1400AN", 512, 528) },
-};
-
-/*
- * This code converts an address to the Default Address Mode, that has non
- * power of two page sizes. We must support this mode because it is the default
- * mode supported by Xilinx tools, it can access the whole flash area and
- * changing over to the Power-of-two mode is irreversible and corrupts the
- * original data.
- * Addr can safely be unsigned int, the biggest S3AN device is smaller than
- * 4 MiB.
- */
-static u32 s3an_nor_convert_addr(struct spi_nor *nor, u32 addr)
-{
- u32 page_size = nor->params->page_size;
- u32 offset, page;
-
- offset = addr % page_size;
- page = addr / page_size;
- page <<= (page_size > 512) ? 10 : 9;
-
- return page | offset;
-}
-
-/**
- * xilinx_nor_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 xilinx_nor_read_sr(struct spi_nor *nor, u8 *sr)
-{
- int ret;
-
- if (nor->spimem) {
- struct spi_mem_op op = XILINX_RDSR_OP(sr);
-
- 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, XILINX_OP_RDSR, sr,
- 1);
- }
-
- if (ret)
- dev_dbg(nor->dev, "error %d reading SR\n", ret);
-
- return ret;
-}
-
-/**
- * xilinx_nor_sr_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 xilinx_nor_sr_ready(struct spi_nor *nor)
-{
- int ret;
-
- ret = xilinx_nor_read_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)
-{
- u32 page_size;
- int ret;
-
- ret = xilinx_nor_read_sr(nor, nor->bouncebuf);
- if (ret)
- return ret;
-
- 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;
-
- /*
- * This flashes have a page size of 264 or 528 bytes (known as
- * Default addressing mode). It can be changed to a more standard
- * Power of two mode where the page size is 256/512. This comes
- * with a price: there is 3% less of space, the data is corrupted
- * and the page size cannot be changed back to default addressing
- * mode.
- *
- * The current addressing mode can be read from the XRDSR register
- * and should not be changed, because is a destructive operation.
- */
- if (nor->bouncebuf[0] & XSR_PAGESIZE) {
- /* Flash in Power of 2 mode */
- page_size = (nor->params->page_size == 264) ? 256 : 512;
- nor->params->page_size = page_size;
- nor->mtd.writebufsize = page_size;
- nor->params->size = nor->info->size;
- nor->mtd.erasesize = 8 * page_size;
- } else {
- /* Flash in Default addressing mode */
- nor->params->convert_addr = s3an_nor_convert_addr;
- nor->mtd.erasesize = nor->info->sector_size;
- }
-
- return 0;
-}
-
-static int xilinx_nor_late_init(struct spi_nor *nor)
-{
- nor->params->setup = xilinx_nor_setup;
- nor->params->ready = xilinx_nor_sr_ready;
-
- return 0;
-}
-
-static const struct spi_nor_fixups xilinx_nor_fixups = {
- .late_init = xilinx_nor_late_init,
-};
-
-const struct spi_nor_manufacturer spi_nor_xilinx = {
- .name = "xilinx",
- .parts = xilinx_nor_parts,
- .nparts = ARRAY_SIZE(xilinx_nor_parts),
- .fixups = &xilinx_nor_fixups,
-};
--
2.39.2
The Xilinx flashes were the only user of the page sized that were no
power of 2. Support for them were dropped, thus we can also get rid of
the special page size handling for it.
Signed-off-by: Michael Walle <[email protected]>
---
drivers/mtd/spi-nor/core.c | 27 +++++++++++----------------
1 file changed, 11 insertions(+), 16 deletions(-)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index cbe5f92eb0af..fb76e0522665 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2098,7 +2098,7 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
size_t *retlen, const u_char *buf)
{
struct spi_nor *nor = mtd_to_spi_nor(mtd);
- size_t page_offset, page_remain, i;
+ size_t i;
ssize_t ret;
u32 page_size = nor->params->page_size;
@@ -2111,21 +2111,9 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
for (i = 0; i < len; ) {
ssize_t written;
loff_t addr = to + i;
-
- /*
- * If page_size is a power of two, the offset can be quickly
- * calculated with an AND operation. On the other cases we
- * need to do a modulus operation (more expensive).
- */
- if (is_power_of_2(page_size)) {
- page_offset = addr & (page_size - 1);
- } else {
- u64 aux = addr;
-
- page_offset = do_div(aux, page_size);
- }
+ size_t page_offset = addr & (page_size - 1);
/* the size of data remaining on the first page */
- page_remain = min_t(size_t, page_size - page_offset, len - i);
+ size_t page_remain = min_t(size_t, page_size - page_offset, len - i);
addr = spi_nor_convert_addr(nor, addr);
@@ -3054,7 +3042,14 @@ static int spi_nor_init_params(struct spi_nor *nor)
spi_nor_init_params_deprecated(nor);
}
- return spi_nor_late_init_params(nor);
+ ret = spi_nor_late_init_params(nor);
+ if (ret)
+ return ret;
+
+ if (WARN_ON(!is_power_of_2(nor->params->page_size)))
+ return -EINVAL;
+
+ return 0;
}
/** spi_nor_set_octal_dtr() - enable or disable Octal DTR I/O.
--
2.39.2
The evervision FRAM devices are the only user of the NO_FR flag. Drop
the global flag and instead use a manufacturer fixup for the evervision
flashes to drop the fast read support.
Signed-off-by: Michael Walle <[email protected]>
---
Please note, that the fast read opcode will still be set in
spi_nor_init_default_params(), but the selection of the read opcodes
just depends on the mask.
That is also something I want to fix soon: the opcodes can always
be set and the drivers/SFDP will only set the mask. Opcodes then can be
switched between 3b and 4b ones if necessary.
---
drivers/mtd/spi-nor/core.c | 12 +++++-------
drivers/mtd/spi-nor/core.h | 2 --
drivers/mtd/spi-nor/everspin.c | 19 +++++++++++++++----
3 files changed, 20 insertions(+), 13 deletions(-)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index fb76e0522665..65e6531ada0a 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2952,14 +2952,12 @@ static void spi_nor_init_default_params(struct spi_nor *nor)
params->page_size = info->page_size ?: SPI_NOR_DEFAULT_PAGE_SIZE;
params->n_banks = info->n_banks ?: SPI_NOR_DEFAULT_N_BANKS;
- if (!(info->flags & SPI_NOR_NO_FR)) {
- /* Default to Fast Read for DT and non-DT platform devices. */
- params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST;
+ /* Default to Fast Read for DT and non-DT platform devices. */
+ params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST;
- /* Mask out Fast Read if not requested at DT instantiation. */
- if (np && !of_property_read_bool(np, "m25p,fast-read"))
- params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
- }
+ /* Mask out Fast Read if not requested at DT instantiation. */
+ if (np && !of_property_read_bool(np, "m25p,fast-read"))
+ params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
/* (Fast) Read settings. */
params->hwcaps.mask |= SNOR_HWCAPS_READ;
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 072c69b0d06c..9aa7d6399c8a 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -479,7 +479,6 @@ struct spi_nor_id {
* Usually these will power-up in a write-protected
* state.
* SPI_NOR_NO_ERASE: no erase command needed.
- * SPI_NOR_NO_FR: can't do fastread.
* SPI_NOR_QUAD_PP: flash supports Quad Input Page Program.
* SPI_NOR_RWW: flash supports reads while write.
*
@@ -528,7 +527,6 @@ struct flash_info {
#define SPI_NOR_BP3_SR_BIT6 BIT(4)
#define SPI_NOR_SWP_IS_VOLATILE BIT(5)
#define SPI_NOR_NO_ERASE BIT(6)
-#define SPI_NOR_NO_FR BIT(7)
#define SPI_NOR_QUAD_PP BIT(8)
#define SPI_NOR_RWW BIT(9)
diff --git a/drivers/mtd/spi-nor/everspin.c b/drivers/mtd/spi-nor/everspin.c
index 5f321e24ae7d..0720a61947e7 100644
--- a/drivers/mtd/spi-nor/everspin.c
+++ b/drivers/mtd/spi-nor/everspin.c
@@ -14,28 +14,39 @@ static const struct flash_info everspin_nor_parts[] = {
.size = SZ_16K,
.sector_size = SZ_16K,
.addr_nbytes = 2,
- .flags = SPI_NOR_NO_ERASE | SPI_NOR_NO_FR,
+ .flags = SPI_NOR_NO_ERASE,
}, {
.name = "mr25h256",
.size = SZ_32K,
.sector_size = SZ_32K,
.addr_nbytes = 2,
- .flags = SPI_NOR_NO_ERASE | SPI_NOR_NO_FR,
+ .flags = SPI_NOR_NO_ERASE,
}, {
.name = "mr25h10",
.size = SZ_128K,
.sector_size = SZ_128K,
- .flags = SPI_NOR_NO_ERASE | SPI_NOR_NO_FR,
+ .flags = SPI_NOR_NO_ERASE,
}, {
.name = "mr25h40",
.size = SZ_512K,
.sector_size = SZ_512K,
- .flags = SPI_NOR_NO_ERASE | SPI_NOR_NO_FR,
+ .flags = SPI_NOR_NO_ERASE,
}
};
+static void evervision_nor_default_init(struct spi_nor *nor)
+{
+ /* Everspin FRAMs don't support the fast read opcode. */
+ nor->params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
+}
+
+static const struct spi_nor_fixups evervision_nor_fixups = {
+ .default_init = evervision_nor_default_init,
+};
+
const struct spi_nor_manufacturer spi_nor_everspin = {
.name = "everspin",
.parts = everspin_nor_parts,
.nparts = ARRAY_SIZE(everspin_nor_parts),
+ .fixups = &evervision_nor_fixups,
};
--
2.39.2
There is no flash driver using that hook. The original intention was to
let the driver configure special requirements like page size an opcodes.
This is already possible by other means and it is unlikely a flash will
overwrite the (more or less complex) setup function.
Signed-off-by: Michael Walle <[email protected]>
---
drivers/mtd/spi-nor/core.c | 105 ++++++++++++++++---------------------
drivers/mtd/spi-nor/core.h | 5 --
2 files changed, 45 insertions(+), 65 deletions(-)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 65e6531ada0a..bbfef7b3997f 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2568,8 +2568,51 @@ static int spi_nor_select_erase(struct spi_nor *nor)
return 0;
}
-static int spi_nor_default_setup(struct spi_nor *nor,
- const struct spi_nor_hwcaps *hwcaps)
+static int spi_nor_set_addr_nbytes(struct spi_nor *nor)
+{
+ if (nor->params->addr_nbytes) {
+ nor->addr_nbytes = nor->params->addr_nbytes;
+ } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
+ /*
+ * In 8D-8D-8D mode, one byte takes half a cycle to transfer. So
+ * in this protocol an odd addr_nbytes cannot be used because
+ * then the address phase would only span a cycle and a half.
+ * Half a cycle would be left over. We would then have to start
+ * the dummy phase in the middle of a cycle and so too the data
+ * phase, and we will end the transaction with half a cycle left
+ * over.
+ *
+ * Force all 8D-8D-8D flashes to use an addr_nbytes of 4 to
+ * avoid this situation.
+ */
+ nor->addr_nbytes = 4;
+ } else if (nor->info->addr_nbytes) {
+ nor->addr_nbytes = nor->info->addr_nbytes;
+ } else {
+ nor->addr_nbytes = 3;
+ }
+
+ if (nor->addr_nbytes == 3 && nor->params->size > 0x1000000) {
+ /* enable 4-byte addressing if the device exceeds 16MiB */
+ nor->addr_nbytes = 4;
+ }
+
+ if (nor->addr_nbytes > SPI_NOR_MAX_ADDR_NBYTES) {
+ dev_dbg(nor->dev, "The number of address bytes is too large: %u\n",
+ nor->addr_nbytes);
+ return -EINVAL;
+ }
+
+ /* Set 4byte opcodes when possible. */
+ if (nor->addr_nbytes == 4 && nor->flags & SNOR_F_4B_OPCODES &&
+ !(nor->flags & SNOR_F_HAS_4BAIT))
+ spi_nor_set_4byte_opcodes(nor);
+
+ return 0;
+}
+
+static int spi_nor_setup(struct spi_nor *nor,
+ const struct spi_nor_hwcaps *hwcaps)
{
struct spi_nor_flash_parameter *params = nor->params;
u32 ignored_mask, shared_mask;
@@ -2626,64 +2669,6 @@ static int spi_nor_default_setup(struct spi_nor *nor,
return err;
}
- return 0;
-}
-
-static int spi_nor_set_addr_nbytes(struct spi_nor *nor)
-{
- if (nor->params->addr_nbytes) {
- nor->addr_nbytes = nor->params->addr_nbytes;
- } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
- /*
- * In 8D-8D-8D mode, one byte takes half a cycle to transfer. So
- * in this protocol an odd addr_nbytes cannot be used because
- * then the address phase would only span a cycle and a half.
- * Half a cycle would be left over. We would then have to start
- * the dummy phase in the middle of a cycle and so too the data
- * phase, and we will end the transaction with half a cycle left
- * over.
- *
- * Force all 8D-8D-8D flashes to use an addr_nbytes of 4 to
- * avoid this situation.
- */
- nor->addr_nbytes = 4;
- } else if (nor->info->addr_nbytes) {
- nor->addr_nbytes = nor->info->addr_nbytes;
- } else {
- nor->addr_nbytes = 3;
- }
-
- if (nor->addr_nbytes == 3 && nor->params->size > 0x1000000) {
- /* enable 4-byte addressing if the device exceeds 16MiB */
- nor->addr_nbytes = 4;
- }
-
- if (nor->addr_nbytes > SPI_NOR_MAX_ADDR_NBYTES) {
- dev_dbg(nor->dev, "The number of address bytes is too large: %u\n",
- nor->addr_nbytes);
- return -EINVAL;
- }
-
- /* Set 4byte opcodes when possible. */
- if (nor->addr_nbytes == 4 && nor->flags & SNOR_F_4B_OPCODES &&
- !(nor->flags & SNOR_F_HAS_4BAIT))
- spi_nor_set_4byte_opcodes(nor);
-
- return 0;
-}
-
-static int spi_nor_setup(struct spi_nor *nor,
- const struct spi_nor_hwcaps *hwcaps)
-{
- int ret;
-
- if (nor->params->setup)
- ret = nor->params->setup(nor, hwcaps);
- else
- ret = spi_nor_default_setup(nor, hwcaps);
- if (ret)
- return ret;
-
return spi_nor_set_addr_nbytes(nor);
}
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 9aa7d6399c8a..8552e31b1b07 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -369,10 +369,6 @@ struct spi_nor_otp {
* @convert_addr: converts an absolute address into something the flash
* will understand. Particularly useful when pagesize is
* not a power-of-2.
- * @setup: (optional) configures the SPI NOR memory. Useful for
- * SPI NOR flashes that have peculiarities to the SPI NOR
- * standard e.g. different opcodes, specific address
- * calculation, page size, etc.
* @ready: (optional) flashes might use a different mechanism
* than reading the status register to indicate they
* are ready for a new command
@@ -404,7 +400,6 @@ struct spi_nor_flash_parameter {
int (*quad_enable)(struct spi_nor *nor);
int (*set_4byte_addr_mode)(struct spi_nor *nor, bool enable);
u32 (*convert_addr)(struct spi_nor *nor, u32 addr);
- int (*setup)(struct spi_nor *nor, const struct spi_nor_hwcaps *hwcaps);
int (*ready)(struct spi_nor *nor);
const struct spi_nor_locking_ops *locking_ops;
--
2.39.2
Rework spi_nor_get_flash_info() to make it look more straight forward
and esp. don't return early. The latter is a preparation to check for
deprecated flashes.
Signed-off-by: Michael Walle <[email protected]>
---
drivers/mtd/spi-nor/core.c | 45 ++++++++++++++++++--------------------
1 file changed, 21 insertions(+), 24 deletions(-)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index bbfef7b3997f..58d310427d35 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3311,39 +3311,36 @@ 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 *jinfo = NULL, *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)
- return spi_nor_detect(nor);
-
/*
- * If caller has specified name of flash model that can normally be
- * detected using JEDEC, let's verify it.
+ * Auto-detect if chip name wasn't specified or not found, or the chip
+ * has an ID. If the chip supposedly has an ID, we also do an
+ * auto-detection to compare it later.
*/
- if (name && info->id) {
- const struct flash_info *jinfo;
-
+ if (!info || info->id) {
jinfo = spi_nor_detect(nor);
- if (IS_ERR(jinfo)) {
+ if (IS_ERR(jinfo))
return jinfo;
- } else if (jinfo != info) {
- /*
- * JEDEC knows better, so overwrite platform ID. We
- * can't trust partitions any longer, but we'll let
- * mtd apply them anyway, since some partitions may be
- * marked read-only, and we don't want to loose that
- * information, even if it's not 100% accurate.
- */
- dev_warn(nor->dev, "found %s, expected %s\n",
- jinfo->name, info->name);
- info = jinfo;
- }
}
- return info;
+ /*
+ * If caller has specified name of flash model that can normally be
+ * detected using JEDEC, let's verify it.
+ */
+ if (info && jinfo && jinfo != info)
+ dev_warn(nor->dev, "found %s, expected %s\n",
+ jinfo->name, info->name);
+
+ /*
+ * JEDEC knows better, so overwrite platform ID. We can't trust
+ * partitions any longer, but we'll let mtd apply them anyway, since
+ * some partitions may be marked read-only, and we don't want to loose
+ * that information, even if it's not 100% accurate.
+ */
+ return jinfo ?: info;
}
static u32
--
2.39.2
SPI-NOR will automatically detect the attached flash device most of the
time. We cannot easily find out if boards are using a given flash.
Therefore, introduce a (temporary) flag to display a message on boot if
support for a given flash device is scheduled to be removed in the
future.
Signed-off-by: Michael Walle <[email protected]>
---
drivers/mtd/spi-nor/core.c | 12 ++++++++++++
drivers/mtd/spi-nor/core.h | 1 +
2 files changed, 13 insertions(+)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 58d310427d35..a294eef2e34a 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3312,6 +3312,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
const char *name)
{
const struct flash_info *jinfo = NULL, *info = NULL;
+ const char *deprecated = NULL;
if (name)
info = spi_nor_match_name(nor, name);
@@ -3326,6 +3327,17 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
return jinfo;
}
+ if (info && (info->flags & SPI_NOR_DEPRECATED))
+ deprecated = info->name;
+ else if (jinfo && (jinfo->flags & SPI_NOR_DEPRECATED))
+ deprecated = jinfo->name;
+
+ if (deprecated)
+ pr_warn("Your board or device tree is using a SPI NOR flash (%s) with\n"
+ "deprecated driver support. It will be removed in future kernel\n"
+ "version. If you feel this shouldn't be the case, please contact\n"
+ "us at [email protected]\n", deprecated);
+
/*
* If caller has specified name of flash model that can normally be
* detected using JEDEC, let's verify it.
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 8552e31b1b07..0317d8e253f4 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -524,6 +524,7 @@ struct flash_info {
#define SPI_NOR_NO_ERASE BIT(6)
#define SPI_NOR_QUAD_PP BIT(8)
#define SPI_NOR_RWW BIT(9)
+#define SPI_NOR_DEPRECATED BIT(15)
u8 no_sfdp_flags;
#define SPI_NOR_SKIP_SFDP BIT(0)
--
2.39.2
On 4/12/24 14:44, Michael Walle wrote:
> These flashes are kind of an oddball for the very old Xilinx Spartan 3
> FPGAs to store their bitstream. More importantly, they reuse the Atmel
> JEDEC manufacturer ID and in fact the at45db081d already blocks the use
> of the 3S700AN flash chip. It's time to sunset support for these
> flashes.
>
> Signed-off-by: Michael Walle <[email protected]>
Good, I'll let Pratyush review the details:
Acked-by: Tudor Ambarus <[email protected]>
Please resend once you get enough feedback and include in To: all the
contributors to this driver.
Thanks!
I'll let Pratyush review the details:
Acked-by: Tudor Ambarus <[email protected]>
On 4/12/24 14:44, Michael Walle wrote:
> The evervision FRAM devices are the only user of the NO_FR flag. Drop
> the global flag and instead use a manufacturer fixup for the evervision
> flashes to drop the fast read support.
>
Don't we want to get rid of FRAMs from SPI NOR? Why the dance then?
On 4/12/24 14:44, Michael Walle wrote:
> There is no flash driver using that hook. The original intention was to
because you just removed its single user, xilinx. Please specify this in
the commit message.
Also, you can get rid of the address conversion thingy.
On Fri Apr 12, 2024 at 3:53 PM CEST, Tudor Ambarus wrote:
>
>
> On 4/12/24 14:44, Michael Walle wrote:
> > These flashes are kind of an oddball for the very old Xilinx Spartan 3
> > FPGAs to store their bitstream. More importantly, they reuse the Atmel
> > JEDEC manufacturer ID and in fact the at45db081d already blocks the use
> > of the 3S700AN flash chip. It's time to sunset support for these
> > flashes.
> >
> > Signed-off-by: Michael Walle <[email protected]>
>
> Good, I'll let Pratyush review the details:
>
> Acked-by: Tudor Ambarus <[email protected]>
>
> Please resend once you get enough feedback and include in To: all the
> contributors to this driver.
For my future reference: This seem so be the only interesting
commit:
commit e99ca98f1d7190c16601b00d0c96212d7c00577d
Author: Ricardo Ribalda <[email protected]>
Date: Fri Dec 2 12:31:44 2016 +0100
mtd: spi-nor: Add support for S3AN spi-nor devices
-michael
On Fri Apr 12, 2024 at 4:00 PM CEST, Tudor Ambarus wrote:
>
>
> On 4/12/24 14:44, Michael Walle wrote:
> > The evervision FRAM devices are the only user of the NO_FR flag. Drop
> > the global flag and instead use a manufacturer fixup for the evervision
> > flashes to drop the fast read support.
> >
>
> Don't we want to get rid of FRAMs from SPI NOR? Why the dance then?
Yes, but it isn't that easy. There are (three?) in-tree users of
these chips. But we can already move all the special handling out of
the core.
-michael
On 4/12/24 14:44, Michael Walle wrote:
> Therefore, introduce a (temporary) flag to display a message on boot if
Fine by me!
Hi,
On Fri, Apr 12 2024, Michael Walle wrote:
> On Fri Apr 12, 2024 at 3:53 PM CEST, Tudor Ambarus wrote:
>>
>>
>> On 4/12/24 14:44, Michael Walle wrote:
>> > These flashes are kind of an oddball for the very old Xilinx Spartan 3
>> > FPGAs to store their bitstream. More importantly, they reuse the Atmel
>> > JEDEC manufacturer ID and in fact the at45db081d already blocks the use
>> > of the 3S700AN flash chip. It's time to sunset support for these
>> > flashes.
>> >
>> > Signed-off-by: Michael Walle <[email protected]>
>>
>> Good, I'll let Pratyush review the details:
>>
>> Acked-by: Tudor Ambarus <[email protected]>
>>
>> Please resend once you get enough feedback and include in To: all the
>> contributors to this driver.
+1. It would be good to hear from past contributors to see if they still
use it.
Also, I think we should let this patch cook in linux-next for some time
before merging to give people a chance to complain if we break
something. Since now there isn't much time left until the SPI NOR pull
request goes out (around 2 weeks I reckon), it would probably make sense
to merge it in the next cycle.
Otherwise, the patch looks good to me.
Reviewed-by: Pratyush Yadav <[email protected]>
>
>
> For my future reference: This seem so be the only interesting
> commit:
>
> commit e99ca98f1d7190c16601b00d0c96212d7c00577d
> Author: Ricardo Ribalda <[email protected]>
> Date: Fri Dec 2 12:31:44 2016 +0100
>
> mtd: spi-nor: Add support for S3AN spi-nor devices
>
> -michael
--
Regards,
Pratyush Yadav
Hi,
On Fri, Apr 12 2024, Michael Walle wrote:
> The Xilinx flashes were the only user of the page sized that were no
> power of 2. Support for them were dropped, thus we can also get rid of
> the special page size handling for it.
Looks like the Xilinx flashes are the only users of
flash_info->page_size, and params->convert_addr (along with
spi_nor_convert_addr()) so those should also be dropped.
The patch looks good to me otherwise.
[...]
--
Regards,
Pratyush Yadav
On 4/15/24 16:26, Pratyush Yadav wrote:
> Also, I think we should let this patch cook in linux-next for some time
> before merging to give people a chance to complain if we break
> something. Since now there isn't much time left until the SPI NOR pull
> request goes out (around 2 weeks I reckon), it would probably make sense
> to merge it in the next cycle.
sounds good to me!
On 4/12/24 15:03, Michael Walle wrote:
> On Fri Apr 12, 2024 at 4:00 PM CEST, Tudor Ambarus wrote:
>>
>>
>> On 4/12/24 14:44, Michael Walle wrote:
>>> The evervision FRAM devices are the only user of the NO_FR flag. Drop
>>> the global flag and instead use a manufacturer fixup for the evervision
>>> flashes to drop the fast read support.
>>>
>>
>> Don't we want to get rid of FRAMs from SPI NOR? Why the dance then?
>
> Yes, but it isn't that easy. There are (three?) in-tree users of
> these chips. But we can already move all the special handling out of
> the core.
>
Okay.
On 4/12/24 14:43, Michael Walle wrote:
> It's time for some spring cleaning. Remove the oddball xilinx
> flashes with non-power-of-2 page sizes.
> Remove the .setup() callback, only the default callback is ever
> used and it is unlikely there is need for a custom setup.
>
> Finally, the last patch is a proposal how to deprecate flashes,
> which are just detected by their id. We cannot really find out if
> there are boards out there which are using a particular flash. Thus,
> as a first step, we can print a warning during kernel startup. As a
> second step we might introduce a kernel config option to actually
> disable the flashes which has the deprecated flag.
How many FRAMs/MRAMs are in SPI NOR? Can you list them please? I don't
remember anyone bringing topics about everspin and the cypress thingy
was a mistake. I'd like to get an idea whether it's fine to remove the
FRAM/MRAM from SPI NOR without doing these preparation steps and waiting
for another year.
Hi Michael,
On Fri, Apr 12 2024, Michael Walle wrote:
> The evervision FRAM devices are the only user of the NO_FR flag. Drop
> the global flag and instead use a manufacturer fixup for the evervision
> flashes to drop the fast read support.
>
> Signed-off-by: Michael Walle <[email protected]>
> ---
> Please note, that the fast read opcode will still be set in
> spi_nor_init_default_params(), but the selection of the read opcodes
> just depends on the mask.
Since that is the case now, might as well drop the
if (params->hwcaps.mask & SNOR_HWCAPS_READ_FAST)
in spi_nor_init_default_params().
>
> That is also something I want to fix soon: the opcodes can always
> be set and the drivers/SFDP will only set the mask. Opcodes then can be
> switched between 3b and 4b ones if necessary.
> ---
> drivers/mtd/spi-nor/core.c | 12 +++++-------
> drivers/mtd/spi-nor/core.h | 2 --
> drivers/mtd/spi-nor/everspin.c | 19 +++++++++++++++----
> 3 files changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index fb76e0522665..65e6531ada0a 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2952,14 +2952,12 @@ static void spi_nor_init_default_params(struct spi_nor *nor)
> params->page_size = info->page_size ?: SPI_NOR_DEFAULT_PAGE_SIZE;
> params->n_banks = info->n_banks ?: SPI_NOR_DEFAULT_N_BANKS;
>
> - if (!(info->flags & SPI_NOR_NO_FR)) {
> - /* Default to Fast Read for DT and non-DT platform devices. */
> - params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST;
> + /* Default to Fast Read for DT and non-DT platform devices. */
> + params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST;
>
> - /* Mask out Fast Read if not requested at DT instantiation. */
> - if (np && !of_property_read_bool(np, "m25p,fast-read"))
> - params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
> - }
> + /* Mask out Fast Read if not requested at DT instantiation. */
> + if (np && !of_property_read_bool(np, "m25p,fast-read"))
> + params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
Nit: move this above where SNOR_CMD_READ_FAST is set up.
Also, I think this is a bit clearer:
/* Default to Fast Read for non-DT and enable it if requested by DT. */
if (!np || of_property_read_bool(np, "m25p,fast-read"))
params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST;
>
> /* (Fast) Read settings. */
> params->hwcaps.mask |= SNOR_HWCAPS_READ;
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 072c69b0d06c..9aa7d6399c8a 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -479,7 +479,6 @@ struct spi_nor_id {
> * Usually these will power-up in a write-protected
> * state.
> * SPI_NOR_NO_ERASE: no erase command needed.
> - * SPI_NOR_NO_FR: can't do fastread.
> * SPI_NOR_QUAD_PP: flash supports Quad Input Page Program.
> * SPI_NOR_RWW: flash supports reads while write.
> *
> @@ -528,7 +527,6 @@ struct flash_info {
> #define SPI_NOR_BP3_SR_BIT6 BIT(4)
> #define SPI_NOR_SWP_IS_VOLATILE BIT(5)
> #define SPI_NOR_NO_ERASE BIT(6)
> -#define SPI_NOR_NO_FR BIT(7)
> #define SPI_NOR_QUAD_PP BIT(8)
> #define SPI_NOR_RWW BIT(9)
Move the other bits up since the slot is now free.
>
> diff --git a/drivers/mtd/spi-nor/everspin.c b/drivers/mtd/spi-nor/everspin.c
> index 5f321e24ae7d..0720a61947e7 100644
> --- a/drivers/mtd/spi-nor/everspin.c
> +++ b/drivers/mtd/spi-nor/everspin.c
> @@ -14,28 +14,39 @@ static const struct flash_info everspin_nor_parts[] = {
> .size = SZ_16K,
> .sector_size = SZ_16K,
> .addr_nbytes = 2,
> - .flags = SPI_NOR_NO_ERASE | SPI_NOR_NO_FR,
> + .flags = SPI_NOR_NO_ERASE,
> }, {
> .name = "mr25h256",
> .size = SZ_32K,
> .sector_size = SZ_32K,
> .addr_nbytes = 2,
> - .flags = SPI_NOR_NO_ERASE | SPI_NOR_NO_FR,
> + .flags = SPI_NOR_NO_ERASE,
> }, {
> .name = "mr25h10",
> .size = SZ_128K,
> .sector_size = SZ_128K,
> - .flags = SPI_NOR_NO_ERASE | SPI_NOR_NO_FR,
> + .flags = SPI_NOR_NO_ERASE,
> }, {
> .name = "mr25h40",
> .size = SZ_512K,
> .sector_size = SZ_512K,
> - .flags = SPI_NOR_NO_ERASE | SPI_NOR_NO_FR,
> + .flags = SPI_NOR_NO_ERASE,
> }
> };
>
> +static void evervision_nor_default_init(struct spi_nor *nor)
> +{
> + /* Everspin FRAMs don't support the fast read opcode. */
> + nor->params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
> +}
> +
> +static const struct spi_nor_fixups evervision_nor_fixups = {
> + .default_init = evervision_nor_default_init,
> +};
> +
> const struct spi_nor_manufacturer spi_nor_everspin = {
> .name = "everspin",
> .parts = everspin_nor_parts,
> .nparts = ARRAY_SIZE(everspin_nor_parts),
> + .fixups = &evervision_nor_fixups,
> };
--
Regards,
Pratyush Yadav
Hi,
On Fri, Apr 12 2024, Michael Walle wrote:
> Rework spi_nor_get_flash_info() to make it look more straight forward
> and esp. don't return early. The latter is a preparation to check for
> deprecated flashes.
Looks much nicer now! Though I did spend around 15 minutes wondering if
it can be made even simpler, but I can't come up with anything better.
Reviewed-by: Pratyush Yadav <[email protected]>
[...]
--
Regards,
Pratyush Yadav
On Fri, Apr 12 2024, Michael Walle wrote:
> SPI-NOR will automatically detect the attached flash device most of the
> time. We cannot easily find out if boards are using a given flash.
> Therefore, introduce a (temporary) flag to display a message on boot if
Why temporary? There will always be a need to deprecate one flash or
another. Might as well keep the flag around.
Also, this patch/series does not add any users of the deprecated flag.
If you have some flashes in mind, it would be good to add them to the
patch/series.
I like the idea in general. Do you think we should also print a rough
date for the deprecation as well?
> support for a given flash device is scheduled to be removed in the
> future.
>
> Signed-off-by: Michael Walle <[email protected]>
> ---
> drivers/mtd/spi-nor/core.c | 12 ++++++++++++
> drivers/mtd/spi-nor/core.h | 1 +
> 2 files changed, 13 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 58d310427d35..a294eef2e34a 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3312,6 +3312,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
> const char *name)
> {
> const struct flash_info *jinfo = NULL, *info = NULL;
> + const char *deprecated = NULL;
>
> if (name)
> info = spi_nor_match_name(nor, name);
> @@ -3326,6 +3327,17 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
> return jinfo;
> }
>
> + if (info && (info->flags & SPI_NOR_DEPRECATED))
> + deprecated = info->name;
> + else if (jinfo && (jinfo->flags & SPI_NOR_DEPRECATED))
> + deprecated = jinfo->name;
> +
> + if (deprecated)
> + pr_warn("Your board or device tree is using a SPI NOR flash (%s) with\n"
> + "deprecated driver support. It will be removed in future kernel\n"
Nit: "removed in a future kernel version"
> + "version. If you feel this shouldn't be the case, please contact\n"
> + "us at [email protected]\n", deprecated);
> +
Hmm, this isn't so nice. I'd suggest doing something like:
/*
* If caller has specified name of flash model that can normally be
* ...
*/
info = jinfo ?: info;
if (info->flags & SPI_NOR_DEPRECATED)
pr_warn(...);
return info;
> /*
> * If caller has specified name of flash model that can normally be
> * detected using JEDEC, let's verify it.
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 8552e31b1b07..0317d8e253f4 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -524,6 +524,7 @@ struct flash_info {
> #define SPI_NOR_NO_ERASE BIT(6)
> #define SPI_NOR_QUAD_PP BIT(8)
> #define SPI_NOR_RWW BIT(9)
> +#define SPI_NOR_DEPRECATED BIT(15)
If you do agree with my suggestion of making it permanent, would it make
more sense to make it BIT(10) instead. Or BIT(9) once you move up the
others because we no longer have BIT(7).
>
> u8 no_sfdp_flags;
> #define SPI_NOR_SKIP_SFDP BIT(0)
--
Regards,
Pratyush Yadav
Hi,
On Wed Apr 17, 2024 at 3:39 PM CEST, Pratyush Yadav wrote:
> On Fri, Apr 12 2024, Michael Walle wrote:
>
> > The evervision FRAM devices are the only user of the NO_FR flag. Drop
> > the global flag and instead use a manufacturer fixup for the evervision
> > flashes to drop the fast read support.
> >
> > Signed-off-by: Michael Walle <[email protected]>
> > ---
> > Please note, that the fast read opcode will still be set in
> > spi_nor_init_default_params(), but the selection of the read opcodes
> > just depends on the mask.
>
> Since that is the case now, might as well drop the
>
> if (params->hwcaps.mask & SNOR_HWCAPS_READ_FAST)
>
> in spi_nor_init_default_params().
I want to address that in another patch where I'll do that for all
the opcodes. Just doing it for the fast read looks odd.
> > That is also something I want to fix soon: the opcodes can always
> > be set and the drivers/SFDP will only set the mask. Opcodes then can be
> > switched between 3b and 4b ones if necessary.
> > ---
> > drivers/mtd/spi-nor/core.c | 12 +++++-------
> > drivers/mtd/spi-nor/core.h | 2 --
> > drivers/mtd/spi-nor/everspin.c | 19 +++++++++++++++----
> > 3 files changed, 20 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index fb76e0522665..65e6531ada0a 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -2952,14 +2952,12 @@ static void spi_nor_init_default_params(struct spi_nor *nor)
> > params->page_size = info->page_size ?: SPI_NOR_DEFAULT_PAGE_SIZE;
> > params->n_banks = info->n_banks ?: SPI_NOR_DEFAULT_N_BANKS;
> >
> > - if (!(info->flags & SPI_NOR_NO_FR)) {
> > - /* Default to Fast Read for DT and non-DT platform devices. */
> > - params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST;
> > + /* Default to Fast Read for DT and non-DT platform devices. */
> > + params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST;
> >
> > - /* Mask out Fast Read if not requested at DT instantiation. */
> > - if (np && !of_property_read_bool(np, "m25p,fast-read"))
> > - params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
> > - }
> > + /* Mask out Fast Read if not requested at DT instantiation. */
> > + if (np && !of_property_read_bool(np, "m25p,fast-read"))
> > + params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
>
> Nit: move this above where SNOR_CMD_READ_FAST is set up.
>
> Also, I think this is a bit clearer:
>
> /* Default to Fast Read for non-DT and enable it if requested by DT. */
> if (!np || of_property_read_bool(np, "m25p,fast-read"))
> params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST;
Will do.
> > /* (Fast) Read settings. */
> > params->hwcaps.mask |= SNOR_HWCAPS_READ;
> > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> > index 072c69b0d06c..9aa7d6399c8a 100644
> > --- a/drivers/mtd/spi-nor/core.h
> > +++ b/drivers/mtd/spi-nor/core.h
> > @@ -479,7 +479,6 @@ struct spi_nor_id {
> > * Usually these will power-up in a write-protected
> > * state.
> > * SPI_NOR_NO_ERASE: no erase command needed.
> > - * SPI_NOR_NO_FR: can't do fastread.
> > * SPI_NOR_QUAD_PP: flash supports Quad Input Page Program.
> > * SPI_NOR_RWW: flash supports reads while write.
> > *
> > @@ -528,7 +527,6 @@ struct flash_info {
> > #define SPI_NOR_BP3_SR_BIT6 BIT(4)
> > #define SPI_NOR_SWP_IS_VOLATILE BIT(5)
> > #define SPI_NOR_NO_ERASE BIT(6)
> > -#define SPI_NOR_NO_FR BIT(7)
> > #define SPI_NOR_QUAD_PP BIT(8)
> > #define SPI_NOR_RWW BIT(9)
>
> Move the other bits up since the slot is now free.
Mhh can't decide what's better here. On one hand I'd really like to
avoid too much code churn because it's already hard enough to follow
the development using git blame. OTOH, a new flag would need to be
added in between the existing flags. Not sure.. Or we if we run out
of free spots at the end we might get rid of the free slots.
-michael
> > diff --git a/drivers/mtd/spi-nor/everspin.c b/drivers/mtd/spi-nor/everspin.c
> > index 5f321e24ae7d..0720a61947e7 100644
> > --- a/drivers/mtd/spi-nor/everspin.c
> > +++ b/drivers/mtd/spi-nor/everspin.c
> > @@ -14,28 +14,39 @@ static const struct flash_info everspin_nor_parts[] = {
> > .size = SZ_16K,
> > .sector_size = SZ_16K,
> > .addr_nbytes = 2,
> > - .flags = SPI_NOR_NO_ERASE | SPI_NOR_NO_FR,
> > + .flags = SPI_NOR_NO_ERASE,
> > }, {
> > .name = "mr25h256",
> > .size = SZ_32K,
> > .sector_size = SZ_32K,
> > .addr_nbytes = 2,
> > - .flags = SPI_NOR_NO_ERASE | SPI_NOR_NO_FR,
> > + .flags = SPI_NOR_NO_ERASE,
> > }, {
> > .name = "mr25h10",
> > .size = SZ_128K,
> > .sector_size = SZ_128K,
> > - .flags = SPI_NOR_NO_ERASE | SPI_NOR_NO_FR,
> > + .flags = SPI_NOR_NO_ERASE,
> > }, {
> > .name = "mr25h40",
> > .size = SZ_512K,
> > .sector_size = SZ_512K,
> > - .flags = SPI_NOR_NO_ERASE | SPI_NOR_NO_FR,
> > + .flags = SPI_NOR_NO_ERASE,
> > }
> > };
> >
> > +static void evervision_nor_default_init(struct spi_nor *nor)
> > +{
> > + /* Everspin FRAMs don't support the fast read opcode. */
> > + nor->params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
> > +}
> > +
> > +static const struct spi_nor_fixups evervision_nor_fixups = {
> > + .default_init = evervision_nor_default_init,
> > +};
> > +
> > const struct spi_nor_manufacturer spi_nor_everspin = {
> > .name = "everspin",
> > .parts = everspin_nor_parts,
> > .nparts = ARRAY_SIZE(everspin_nor_parts),
> > + .fixups = &evervision_nor_fixups,
> > };
Hi,
On Wed Apr 17, 2024 at 4:36 PM CEST, Pratyush Yadav wrote:
> On Fri, Apr 12 2024, Michael Walle wrote:
>
> > SPI-NOR will automatically detect the attached flash device most of the
> > time. We cannot easily find out if boards are using a given flash.
> > Therefore, introduce a (temporary) flag to display a message on boot if
>
> Why temporary? There will always be a need to deprecate one flash or
> another. Might as well keep the flag around.
Mh, yes I agree. That also means that this flag will not have any
users (most) of the time (hopefully ;) ).
> Also, this patch/series does not add any users of the deprecated flag.
> If you have some flashes in mind, it would be good to add them to the
> patch/series.
This is just an RFC to see if whether you Tudor agree with me :) But
I was about to add it to the evervision/cypress FRAMs.
> I like the idea in general. Do you think we should also print a rough
> date for the deprecation as well?
Might make sense, any suggestions?
> > support for a given flash device is scheduled to be removed in the
> > future.
> >
> > Signed-off-by: Michael Walle <[email protected]>
> > ---
> > drivers/mtd/spi-nor/core.c | 12 ++++++++++++
> > drivers/mtd/spi-nor/core.h | 1 +
> > 2 files changed, 13 insertions(+)
> >
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index 58d310427d35..a294eef2e34a 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -3312,6 +3312,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
> > const char *name)
> > {
> > const struct flash_info *jinfo = NULL, *info = NULL;
> > + const char *deprecated = NULL;
> >
> > if (name)
> > info = spi_nor_match_name(nor, name);
> > @@ -3326,6 +3327,17 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
> > return jinfo;
> > }
> >
> > + if (info && (info->flags & SPI_NOR_DEPRECATED))
> > + deprecated = info->name;
> > + else if (jinfo && (jinfo->flags & SPI_NOR_DEPRECATED))
> > + deprecated = jinfo->name;
> > +
> > + if (deprecated)
> > + pr_warn("Your board or device tree is using a SPI NOR flash (%s) with\n"
> > + "deprecated driver support. It will be removed in future kernel\n"
>
> Nit: "removed in a future kernel version"
>
> > + "version. If you feel this shouldn't be the case, please contact\n"
> > + "us at [email protected]\n", deprecated);
> > +
>
> Hmm, this isn't so nice. I'd suggest doing something like:
>
> /*
> * If caller has specified name of flash model that can normally be
> * ...
> */
> info = jinfo ?: info;
>
> if (info->flags & SPI_NOR_DEPRECATED)
> pr_warn(...);
Actually, I had that, *but* I was thinking we might only check the
detected flash and not the one specified in the device tree. But
thinking about that again, I guess it makes sense because:
- that's the actually used flash driver
- having jinfo != info will print that other warning, thus this
case is hopefully very unlikely.
>
> return info;
>
> > /*
> > * If caller has specified name of flash model that can normally be
> > * detected using JEDEC, let's verify it.
> > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> > index 8552e31b1b07..0317d8e253f4 100644
> > --- a/drivers/mtd/spi-nor/core.h
> > +++ b/drivers/mtd/spi-nor/core.h
> > @@ -524,6 +524,7 @@ struct flash_info {
> > #define SPI_NOR_NO_ERASE BIT(6)
> > #define SPI_NOR_QUAD_PP BIT(8)
> > #define SPI_NOR_RWW BIT(9)
> > +#define SPI_NOR_DEPRECATED BIT(15)
>
> If you do agree with my suggestion of making it permanent, would it make
> more sense to make it BIT(10) instead. Or BIT(9) once you move up the
> others because we no longer have BIT(7).
Or just BIT(7) and avoid any code churn :)
-michael
>
> >
> > u8 no_sfdp_flags;
> > #define SPI_NOR_SKIP_SFDP BIT(0)
Hi,
On Tue Apr 16, 2024 at 6:57 AM CEST, Tudor Ambarus wrote:
> On 4/12/24 14:43, Michael Walle wrote:
> > It's time for some spring cleaning. Remove the oddball xilinx
> > flashes with non-power-of-2 page sizes.
> > Remove the .setup() callback, only the default callback is ever
> > used and it is unlikely there is need for a custom setup.
> >
> > Finally, the last patch is a proposal how to deprecate flashes,
> > which are just detected by their id. We cannot really find out if
> > there are boards out there which are using a particular flash. Thus,
> > as a first step, we can print a warning during kernel startup. As a
> > second step we might introduce a kernel config option to actually
> > disable the flashes which has the deprecated flag.
>
>
> How many FRAMs/MRAMs are in SPI NOR? Can you list them please? I don't
> remember anyone bringing topics about everspin and the cypress thingy
> was a mistake. I'd like to get an idea whether it's fine to remove the
> FRAM/MRAM from SPI NOR without doing these preparation steps and waiting
> for another year.
As far as I can tell it is just
drivers/mtd/spi-nor/evervision.c:
- mr25h128
- mr25h256
- mr25h10
- mr25h40
and
drivers/mtd/spi-nor/spansion.c
- cy15x104q
Please keep in mind that the evervision also have dt bindings and
spi aliases. So we'd also have to deprecate these ones. Not sure
they can easily be moved over to another driver, esp. because the
kernel interface is different (mtd partitions vs nvmem/eeprom sysfs
file).
There are three in-tree kernel boards using the mr25h frams:
arch/arm/boot/dts/nxp/imx/imx6dl-eckelmann-ci4x10.dts
No clue about that one.
arch/arm/boot/dts/nxp/mxs/imx28-sps1.dts
I've contacted Marek who introduced support for this board and he
said it can likely be removed altogether.
arch/arm/boot/dts/marvell/armada-xp-linksys-mamba.dts
This seem to be openwrt driven (?)
I was thinking about sending patches to migrate them over to the
at25 driver. But again, that also means they'll loose mtd
partitions. Not sure, that fram is used on these boards anyway.
In any case I'd like to start with deprecating the FRAMs in
spi-nor and the DT bindings.
Also, the cy15x104g is rather new...
-michael
Hi,
On Wed Apr 17, 2024 at 5:37 PM CEST, Pratyush Yadav wrote:
> BTW, -M and -C options for git-blame can help you a bit. They can detect
> moved and copied lines, and look for the original one to blame. From man
> git-blame:
I know, did you saw "So You Think You Know Git - FOSDEM 2024", too? ;)
Anyway, at least in netdev unnecessary code churn is frowned upon ;)
-michael
On Wed, Apr 17 2024, Michael Walle wrote:
> Hi,
>
> On Wed Apr 17, 2024 at 3:39 PM CEST, Pratyush Yadav wrote:
>> On Fri, Apr 12 2024, Michael Walle wrote:
>>
>> > The evervision FRAM devices are the only user of the NO_FR flag. Drop
>> > the global flag and instead use a manufacturer fixup for the evervision
>> > flashes to drop the fast read support.
>> >
>> > Signed-off-by: Michael Walle <[email protected]>
>> > ---
>> > Please note, that the fast read opcode will still be set in
>> > spi_nor_init_default_params(), but the selection of the read opcodes
>> > just depends on the mask.
>>
>> Since that is the case now, might as well drop the
>>
>> if (params->hwcaps.mask & SNOR_HWCAPS_READ_FAST)
>>
>> in spi_nor_init_default_params().
>
> I want to address that in another patch where I'll do that for all
> the opcodes. Just doing it for the fast read looks odd.
Okay.
[...]
>> > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>> > index 072c69b0d06c..9aa7d6399c8a 100644
>> > --- a/drivers/mtd/spi-nor/core.h
>> > +++ b/drivers/mtd/spi-nor/core.h
>> > @@ -479,7 +479,6 @@ struct spi_nor_id {
>> > * Usually these will power-up in a write-protected
>> > * state.
>> > * SPI_NOR_NO_ERASE: no erase command needed.
>> > - * SPI_NOR_NO_FR: can't do fastread.
>> > * SPI_NOR_QUAD_PP: flash supports Quad Input Page Program.
>> > * SPI_NOR_RWW: flash supports reads while write.
>> > *
>> > @@ -528,7 +527,6 @@ struct flash_info {
>> > #define SPI_NOR_BP3_SR_BIT6 BIT(4)
>> > #define SPI_NOR_SWP_IS_VOLATILE BIT(5)
>> > #define SPI_NOR_NO_ERASE BIT(6)
>> > -#define SPI_NOR_NO_FR BIT(7)
>> > #define SPI_NOR_QUAD_PP BIT(8)
>> > #define SPI_NOR_RWW BIT(9)
>>
>> Move the other bits up since the slot is now free.
>
> Mhh can't decide what's better here. On one hand I'd really like to
> avoid too much code churn because it's already hard enough to follow
> the development using git blame. OTOH, a new flag would need to be
> added in between the existing flags. Not sure.. Or we if we run out
> of free spots at the end we might get rid of the free slots.
Filling this slot with the new deprecated flag should do the trick then.
BTW, -M and -C options for git-blame can help you a bit. They can detect
moved and copied lines, and look for the original one to blame. From man
git-blame:
-M[<num>]
Detect moved or copied lines within a file. When a commit
moves or copies a block of lines (e.g. the original file has
A and then B, and the commit changes it to B and then A), the
traditional blame algorithm notices only half of the movement
and typically blames the lines that were moved up (i.e. B) to
the parent and assigns blame to the lines that were moved
down (i.e. A) to the child commit. With this option, both
groups of lines are blamed on the parent by running extra
passes of inspection.
<num> is optional but it is the lower bound on the number of
alphanumeric characters that Git must detect as
moving/copying within a file for it to associate those lines
with the parent commit. The default value is 20.
-C[<num>]
In addition to -M, detect lines moved or copied from other
files that were modified in the same commit. This is useful
when you reorganize your program and move code around across
files. When this option is given twice, the command
additionally looks for copies from other files in the commit
that creates the file. When this option is given three times,
the command additionally looks for copies from other files in
any commit.
<num> is optional but it is the lower bound on the number of
alphanumeric characters that Git must detect as
moving/copying between files for it to associate those lines
with the parent commit. And the default value is 40. If there
are more than one -C options given, the <num> argument of the
last -C will take effect.
[...]
--
Regards,
Pratyush Yadav
On Wed, Apr 17 2024, Michael Walle wrote:
> Hi,
>
> On Wed Apr 17, 2024 at 4:36 PM CEST, Pratyush Yadav wrote:
>> On Fri, Apr 12 2024, Michael Walle wrote:
>>
>> > SPI-NOR will automatically detect the attached flash device most of the
>> > time. We cannot easily find out if boards are using a given flash.
>> > Therefore, introduce a (temporary) flag to display a message on boot if
>>
>> Why temporary? There will always be a need to deprecate one flash or
>> another. Might as well keep the flag around.
>
> Mh, yes I agree. That also means that this flag will not have any
> users (most) of the time (hopefully ;) ).
>
>> Also, this patch/series does not add any users of the deprecated flag.
>> If you have some flashes in mind, it would be good to add them to the
>> patch/series.
>
> This is just an RFC to see if whether you Tudor agree with me :) But
> I was about to add it to the evervision/cypress FRAMs.
>
>> I like the idea in general. Do you think we should also print a rough
>> date for the deprecation as well?
>
> Might make sense, any suggestions?
How about a simple string to flash_info?
/**
* struct flash_info - SPI NOR flash_info entry.
* @id: pointer to struct spi_nor_id or NULL, which means "no ID" (mostly
* older chips).
* @name: (obsolete) the name of the flash. Do not set it for new additions.
* @size: the size of the flash in bytes.
* @deprecation_date: The date after which the support for this flash will be
* removed.
* [...]
*/
struct flash_info {
char *name;
const struct spi_nor_id *id;
char *deprecation_date;
[...]
}
And then in everspin.c for example,
{
.name = "mr25h128",
.size = SZ_16K,
.sector_size = SZ_16K,
.addr_nbytes = 2,
.flags = SPI_NOR_NO_ERASE | SPI_NOR_NO_FR,
.deprecation_date = "2025-01-01",
}, {
And in spi_nor_get_flash_info() (changed some wording of the message):
info = jinfo ?: info;
if (info->deprecation_date)
pr_warn("Your board or device tree is using a SPI NOR flash (%s) with\n"
"deprecated driver support. It can be removed in any kernel\n"
"version after %s. If you feel this shouldn't be the case, please contact\n"
"us at [email protected]\n", info->name,
info->deprecation_date);
return info;
This would also remove the need for SPI_NOR_DEPRECATED. But it would
make the flash_info 4 or 8 bytes larger.
What do you think?
>
>> > support for a given flash device is scheduled to be removed in the
>> > future.
>> >
>> > Signed-off-by: Michael Walle <[email protected]>
>> > ---
>> > drivers/mtd/spi-nor/core.c | 12 ++++++++++++
>> > drivers/mtd/spi-nor/core.h | 1 +
>> > 2 files changed, 13 insertions(+)
>> >
>> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> > index 58d310427d35..a294eef2e34a 100644
>> > --- a/drivers/mtd/spi-nor/core.c
>> > +++ b/drivers/mtd/spi-nor/core.c
>> > @@ -3312,6 +3312,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
>> > const char *name)
>> > {
>> > const struct flash_info *jinfo = NULL, *info = NULL;
>> > + const char *deprecated = NULL;
>> >
>> > if (name)
>> > info = spi_nor_match_name(nor, name);
>> > @@ -3326,6 +3327,17 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
>> > return jinfo;
>> > }
>> >
>> > + if (info && (info->flags & SPI_NOR_DEPRECATED))
>> > + deprecated = info->name;
>> > + else if (jinfo && (jinfo->flags & SPI_NOR_DEPRECATED))
>> > + deprecated = jinfo->name;
>> > +
>> > + if (deprecated)
>> > + pr_warn("Your board or device tree is using a SPI NOR flash (%s) with\n"
>> > + "deprecated driver support. It will be removed in future kernel\n"
>>
>> Nit: "removed in a future kernel version"
>>
>> > + "version. If you feel this shouldn't be the case, please contact\n"
>> > + "us at [email protected]\n", deprecated);
>> > +
>>
>> Hmm, this isn't so nice. I'd suggest doing something like:
>>
>> /*
>> * If caller has specified name of flash model that can normally be
>> * ...
>> */
>> info = jinfo ?: info;
>>
>> if (info->flags & SPI_NOR_DEPRECATED)
>> pr_warn(...);
>
> Actually, I had that, *but* I was thinking we might only check the
> detected flash and not the one specified in the device tree. But
> thinking about that again, I guess it makes sense because:
> - that's the actually used flash driver
> - having jinfo != info will print that other warning, thus this
> case is hopefully very unlikely.
>
>>
>> return info;
>>
>> > /*
>> > * If caller has specified name of flash model that can normally be
>> > * detected using JEDEC, let's verify it.
>> > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>> > index 8552e31b1b07..0317d8e253f4 100644
>> > --- a/drivers/mtd/spi-nor/core.h
>> > +++ b/drivers/mtd/spi-nor/core.h
>> > @@ -524,6 +524,7 @@ struct flash_info {
>> > #define SPI_NOR_NO_ERASE BIT(6)
>> > #define SPI_NOR_QUAD_PP BIT(8)
>> > #define SPI_NOR_RWW BIT(9)
>> > +#define SPI_NOR_DEPRECATED BIT(15)
>>
>> If you do agree with my suggestion of making it permanent, would it make
>> more sense to make it BIT(10) instead. Or BIT(9) once you move up the
>> others because we no longer have BIT(7).
>
> Or just BIT(7) and avoid any code churn :)
Yep, that works.
>
> -michael
>
>>
>> >
>> > u8 no_sfdp_flags;
>> > #define SPI_NOR_SKIP_SFDP BIT(0)
>
--
Regards,
Pratyush Yadav
On Wed, Apr 17 2024, Michael Walle wrote:
> Hi,
>
> On Wed Apr 17, 2024 at 5:37 PM CEST, Pratyush Yadav wrote:
>> BTW, -M and -C options for git-blame can help you a bit. They can detect
>> moved and copied lines, and look for the original one to blame. From man
>> git-blame:
>
> I know, did you saw "So You Think You Know Git - FOSDEM 2024", too? ;)
Hahaha, yes :-)
> Anyway, at least in netdev unnecessary code churn is frowned upon ;)
>
> -michael
>
--
Regards,
Pratyush Yadav
Hi,
On Wed Apr 17, 2024 at 5:52 PM CEST, Pratyush Yadav wrote:
> On Wed, Apr 17 2024, Michael Walle wrote:
> > On Wed Apr 17, 2024 at 4:36 PM CEST, Pratyush Yadav wrote:
> >> On Fri, Apr 12 2024, Michael Walle wrote:
> >>
> >> > SPI-NOR will automatically detect the attached flash device most of the
> >> > time. We cannot easily find out if boards are using a given flash.
> >> > Therefore, introduce a (temporary) flag to display a message on boot if
> >>
> >> Why temporary? There will always be a need to deprecate one flash or
> >> another. Might as well keep the flag around.
> >
> > Mh, yes I agree. That also means that this flag will not have any
> > users (most) of the time (hopefully ;) ).
> >
> >> Also, this patch/series does not add any users of the deprecated flag.
> >> If you have some flashes in mind, it would be good to add them to the
> >> patch/series.
> >
> > This is just an RFC to see if whether you Tudor agree with me :) But
> > I was about to add it to the evervision/cypress FRAMs.
> >
> >> I like the idea in general. Do you think we should also print a rough
> >> date for the deprecation as well?
> >
> > Might make sense, any suggestions?
>
> How about a simple string to flash_info?
Ahh, I was rather asking if you already had a time frame in mind.
Besides that, should it be a date or a (future) kernel version?
Roughly about two/three kernel releases?
> /**
> * struct flash_info - SPI NOR flash_info entry.
> * @id: pointer to struct spi_nor_id or NULL, which means "no ID" (mostly
> * older chips).
> * @name: (obsolete) the name of the flash. Do not set it for new additions.
> * @size: the size of the flash in bytes.
> * @deprecation_date: The date after which the support for this flash will be
> * removed.
> * [...]
> */
> struct flash_info {
> char *name;
> const struct spi_nor_id *id;
> char *deprecation_date;
> [...]
> }
>
> And then in everspin.c for example,
>
> {
> .name = "mr25h128",
> .size = SZ_16K,
> .sector_size = SZ_16K,
> .addr_nbytes = 2,
> .flags = SPI_NOR_NO_ERASE | SPI_NOR_NO_FR,
> .deprecation_date = "2025-01-01",
> }, {
>
> And in spi_nor_get_flash_info() (changed some wording of the message):
>
> info = jinfo ?: info;
>
> if (info->deprecation_date)
> pr_warn("Your board or device tree is using a SPI NOR flash (%s) with\n"
> "deprecated driver support. It can be removed in any kernel\n"
> "version after %s. If you feel this shouldn't be the case, please contact\n"
> "us at [email protected]\n", info->name,
> info->deprecation_date);
>
> return info;
>
> This would also remove the need for SPI_NOR_DEPRECATED. But it would
> make the flash_info 4 or 8 bytes larger.
>
> What do you think?
Looks good.
-michael
On Thu, Apr 18 2024, Michael Walle wrote:
> Hi,
>
> On Wed Apr 17, 2024 at 5:52 PM CEST, Pratyush Yadav wrote:
>> On Wed, Apr 17 2024, Michael Walle wrote:
>> > On Wed Apr 17, 2024 at 4:36 PM CEST, Pratyush Yadav wrote:
[...]
>> >> I like the idea in general. Do you think we should also print a rough
>> >> date for the deprecation as well?
>> >
>> > Might make sense, any suggestions?
>>
>> How about a simple string to flash_info?
>
> Ahh, I was rather asking if you already had a time frame in mind.
>
> Besides that, should it be a date or a (future) kernel version?
> Roughly about two/three kernel releases?
I think kernel version is better.
I don't base this on any real data, only on gut feeling, but I think
three kernel releases is a good time. Should add up to about 6-7 months.
[...]
--
Regards,
Pratyush Yadav