Hi,
On Octal DTR flashes like Micron Xcella or Cypress S28 family, reads or
writes cannot start at an odd address in 8D-8D-8D mode. Neither can they
be odd bytes long. Upper layers like filesystems don't know what mode
the flash is in, and hence don't know the read/write address or length
limitations. They might issue reads or writes that leave the flash in an
error state. In fact, using UBIFS on top of the flash was how I first
noticed this problem.
This series fixes that problem by padding the read/write with extra
bytes to make sure the final operation has an even address and length.
More info in patches 5 and 6.
Patches 1-3 fix some existing odd-byte long reads. Patch 4 adds checks
to disallow odd length command/address/dummy/data phases in 8D-8D-8D
mode. Note that this does not restrict the _value_ of the address from
being odd since this is a restriction on the flash, not the protocol
itself.
Patch 4 should go through the SPI tree but I have included it in this
series because if it goes in before patches 1-3, Micron MT35XU and
Cypress S28HS flashes will stop working correctly.
Tested on TI J721E for Micron MT35 and on TI J7200 for Cypress S28.
Changes in v2:
Collect R-bys and cosmetic fixes. No functional changes. See the patches
for detailed changelog.
Pratyush Yadav (6):
mtd: spi-nor: core: use 2 data bytes for template ops
mtd: spi-nor: spansion: write 2 bytes when disabling Octal DTR mode
mtd: spi-nor: micron-st: write 2 bytes when disabling Octal DTR mode
spi: spi-mem: reject partial cycle transfers in 8D-8D-8D mode
mtd: spi-nor: core: avoid odd length/address reads on 8D-8D-8D mode
mtd: spi-nor: core: avoid odd length/address writes in 8D-8D-8D mode
drivers/mtd/spi-nor/core.c | 159 +++++++++++++++++++++++++++++++-
drivers/mtd/spi-nor/micron-st.c | 22 ++++-
drivers/mtd/spi-nor/spansion.c | 18 +++-
drivers/spi/spi-mem.c | 12 ++-
4 files changed, 196 insertions(+), 15 deletions(-)
--
2.30.0
The template ops used in spi_nor_spimem_check_pp() and
spi_nor_spimem_check_readop() currently set the data phase to 1 byte
long. This is problematic for 8D-8D-8D protocol where odd length data
phase is invalid since one cycle transfers 2 bytes and odd number of
bytes would mean half a cycle is left over. This could result in a
controller rejecting the op as "not supported" even though it actually
supports the protocol.
Change the data length to 2 bytes in these templates. One might argue
that this should only be done for 8D-8D-8D operations but when talking
about these templates, there is no functional difference between one and
two bytes, even in STR modes.
Signed-off-by: Pratyush Yadav <[email protected]>
---
(no changes since v1)
drivers/mtd/spi-nor/core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index f6a6ef2d8bd8..d521ca577884 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2143,7 +2143,7 @@ static int spi_nor_spimem_check_readop(struct spi_nor *nor,
struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(read->opcode, 0),
SPI_MEM_OP_ADDR(3, 0, 0),
SPI_MEM_OP_DUMMY(1, 0),
- SPI_MEM_OP_DATA_IN(1, NULL, 0));
+ SPI_MEM_OP_DATA_IN(2, NULL, 0));
spi_nor_spimem_setup_op(nor, &op, read->proto);
@@ -2169,7 +2169,7 @@ static int spi_nor_spimem_check_pp(struct spi_nor *nor,
struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(pp->opcode, 0),
SPI_MEM_OP_ADDR(3, 0, 0),
SPI_MEM_OP_NO_DUMMY,
- SPI_MEM_OP_DATA_OUT(1, NULL, 0));
+ SPI_MEM_OP_DATA_OUT(2, NULL, 0));
spi_nor_spimem_setup_op(nor, &op, pp->proto);
--
2.30.0
The Octal DTR configuration is stored in the CFR5V register. This
register is 1 byte wide. But 1 byte long transactions are not allowed in
8D-8D-8D mode. Since the next byte address does not contain any
register, it is safe to write any value to it. Write a 0 to it.
Signed-off-by: Pratyush Yadav <[email protected]>
---
(no changes since v1)
drivers/mtd/spi-nor/spansion.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index ee82dcd75310..e6bf5c9eee6a 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -65,10 +65,18 @@ static int spi_nor_cypress_octal_dtr_enable(struct spi_nor *nor, bool enable)
if (ret)
return ret;
- if (enable)
- *buf = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN;
- else
- *buf = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS;
+ if (enable) {
+ buf[0] = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN;
+ } else {
+ /*
+ * The register is 1-byte wide, but 1-byte transactions are not
+ * allowed in 8D-8D-8D mode. Since there is no register at the
+ * next location, just initialize the value to 0 and let the
+ * transaction go on.
+ */
+ buf[0] = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS;
+ buf[1] = 0;
+ }
op = (struct spi_mem_op)
SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_ANY_REG, 1),
@@ -76,7 +84,7 @@ static int spi_nor_cypress_octal_dtr_enable(struct spi_nor *nor, bool enable)
SPINOR_REG_CYPRESS_CFR5V,
1),
SPI_MEM_OP_NO_DUMMY,
- SPI_MEM_OP_DATA_OUT(1, buf, 1));
+ SPI_MEM_OP_DATA_OUT(enable ? 1 : 2, buf, 1));
if (!enable)
spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
--
2.30.0
In 8D-8D-8D mode two bytes are transferred per cycle. So an odd number
of bytes cannot be transferred because it would leave a residual half
cycle at the end. Consider such a transfer invalid and reject it.
Signed-off-by: Pratyush Yadav <[email protected]>
Reviewed-by: Mark Brown <[email protected]>
---
Changes in v2:
- Add Mark's R-by (spell corrected).
drivers/spi/spi-mem.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index 1513553e4080..ab9eefbaa1d9 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -162,7 +162,17 @@ static bool spi_mem_check_buswidth(struct spi_mem *mem,
bool spi_mem_dtr_supports_op(struct spi_mem *mem,
const struct spi_mem_op *op)
{
- if (op->cmd.nbytes != 2)
+ if (op->cmd.buswidth == 8 && op->cmd.nbytes % 2)
+ return false;
+
+ if (op->addr.nbytes && op->addr.buswidth == 8 && op->addr.nbytes % 2)
+ return false;
+
+ if (op->dummy.nbytes && op->dummy.buswidth == 8 && op->dummy.nbytes % 2)
+ return false;
+
+ if (op->data.dir != SPI_MEM_NO_DATA &&
+ op->dummy.buswidth == 8 && op->data.nbytes % 2)
return false;
return spi_mem_check_buswidth(mem, op);
--
2.30.0
The Octal DTR configuration is stored in the CFR0V register. This
register is 1 byte wide. But 1 byte long transactions are not allowed in
8D-8D-8D mode. The next byte address contains the CFR1V register, which
contains the number of dummy cycles. This is very fortunate since the
enable path changes the value of this register. Reset the value to its
default when disabling Octal DTR mode. This way, both changes to the
flash state made when enabling can be reverted in one single
transaction.
Signed-off-by: Pratyush Yadav <[email protected]>
---
(no changes since v1)
drivers/mtd/spi-nor/micron-st.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
index c224e59820a1..e49bb2f142b3 100644
--- a/drivers/mtd/spi-nor/micron-st.c
+++ b/drivers/mtd/spi-nor/micron-st.c
@@ -13,6 +13,7 @@
#define SPINOR_OP_MT_WR_ANY_REG 0x81 /* Write volatile register */
#define SPINOR_REG_MT_CFR0V 0x00 /* For setting octal DTR mode */
#define SPINOR_REG_MT_CFR1V 0x01 /* For setting dummy cycles */
+#define SPINOR_REG_MT_CFR1V_DEF 0x1f /* Default dummy cycles */
#define SPINOR_MT_OCT_DTR 0xe7 /* Enable Octal DTR. */
#define SPINOR_MT_EXSPI 0xff /* Enable Extended SPI (default) */
@@ -48,17 +49,28 @@ static int spi_nor_micron_octal_dtr_enable(struct spi_nor *nor, bool enable)
if (ret)
return ret;
- if (enable)
- *buf = SPINOR_MT_OCT_DTR;
- else
- *buf = SPINOR_MT_EXSPI;
+ if (enable) {
+ buf[0] = SPINOR_MT_OCT_DTR;
+ } else {
+ /*
+ * The register is 1-byte wide, but 1-byte transactions are not
+ * allowed in 8D-8D-8D mode. The next register is the dummy
+ * cycle configuration register. Since the transaction needs to
+ * be at least 2 bytes wide, set the next register to its
+ * default value. This also makes sense because the value was
+ * changed when enabling 8D-8D-8D mode, it should be reset when
+ * disabling.
+ */
+ buf[0] = SPINOR_MT_EXSPI;
+ buf[1] = SPINOR_REG_MT_CFR1V_DEF;
+ }
op = (struct spi_mem_op)
SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_MT_WR_ANY_REG, 1),
SPI_MEM_OP_ADDR(enable ? 3 : 4,
SPINOR_REG_MT_CFR0V, 1),
SPI_MEM_OP_NO_DUMMY,
- SPI_MEM_OP_DATA_OUT(1, buf, 1));
+ SPI_MEM_OP_DATA_OUT(enable ? 1 : 2, buf, 1));
if (!enable)
spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
--
2.30.0
On Octal DTR capable flashes like Micron Xcella reads cannot start or
end at an odd address in Octal DTR mode. Extra bytes need to be read at
the start or end to make sure both the start address and length remain
even.
To avoid allocating too much extra memory, thereby putting unnecessary
memory pressure on the system, the temporary buffer containing the extra
padding bytes is capped at PAGE_SIZE bytes. The rest of the 2-byte
aligned part should be read directly in the main buffer.
Signed-off-by: Pratyush Yadav <[email protected]>
Reviewed-by: Michael Walle <[email protected]>
---
Changes in v2:
- Replace semicolon in subject with colon.
- Add a comment that ret < 0 after adjusting for extra bytes is not
possible, and add a WARN_ON() on the condition to make sure it gets
spotted quickly when some change triggers this bug.
- Add Michael's R-by.
drivers/mtd/spi-nor/core.c | 82 +++++++++++++++++++++++++++++++++++++-
1 file changed, 81 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index d521ca577884..a696af6a1b71 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1904,6 +1904,83 @@ static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
return ERR_PTR(-ENODEV);
}
+/*
+ * On Octal DTR capable flashes like Micron Xcella reads cannot start or
+ * end at an odd address in Octal DTR mode. Extra bytes need to be read
+ * at the start or end to make sure both the start address and length
+ * remain even.
+ */
+static int spi_nor_octal_dtr_read(struct spi_nor *nor, loff_t from, size_t len,
+ u_char *buf)
+{
+ u_char *tmp_buf;
+ size_t tmp_len;
+ loff_t start, end;
+ int ret, bytes_read;
+
+ if (IS_ALIGNED(from, 2) && IS_ALIGNED(len, 2))
+ return spi_nor_read_data(nor, from, len, buf);
+ else if (IS_ALIGNED(from, 2) && len > PAGE_SIZE)
+ return spi_nor_read_data(nor, from, round_down(len, PAGE_SIZE),
+ buf);
+
+ tmp_buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
+ if (!tmp_buf)
+ return -ENOMEM;
+
+ start = round_down(from, 2);
+ end = round_up(from + len, 2);
+
+ /*
+ * Avoid allocating too much memory. The requested read length might be
+ * quite large. Allocating a buffer just as large (slightly bigger, in
+ * fact) would put unnecessary memory pressure on the system.
+ *
+ * For example if the read is from 3 to 1M, then this will read from 2
+ * to 4098. The reads from 4098 to 1M will then not need a temporary
+ * buffer so they can proceed as normal.
+ */
+ tmp_len = min_t(size_t, end - start, PAGE_SIZE);
+
+ ret = spi_nor_read_data(nor, start, tmp_len, tmp_buf);
+ if (ret == 0) {
+ ret = -EIO;
+ goto out;
+ }
+ if (ret < 0)
+ goto out;
+
+ /*
+ * More bytes are read than actually requested, but that number can't be
+ * reported to the calling function or it will confuse its calculations.
+ * Calculate how many of the _requested_ bytes were read.
+ */
+ bytes_read = ret;
+
+ if (from != start)
+ ret -= from - start;
+
+ /*
+ * Only account for extra bytes at the end if they were actually read.
+ * For example, if the total length was truncated because of temporary
+ * buffer size limit then the adjustment for the extra bytes at the end
+ * is not needed.
+ */
+ if (start + bytes_read == end)
+ ret -= end - (from + len);
+
+ /* Should not be possible. */
+ if (WARN_ON(ret < 0)) {
+ ret = -EIO;
+ goto out;
+ }
+
+ memcpy(buf, tmp_buf + (from - start), ret);
+out:
+ kfree(tmp_buf);
+ return ret;
+}
+
static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
size_t *retlen, u_char *buf)
{
@@ -1921,7 +1998,10 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
addr = spi_nor_convert_addr(nor, addr);
- ret = spi_nor_read_data(nor, addr, len, buf);
+ if (nor->read_proto == SNOR_PROTO_8_8_8_DTR)
+ ret = spi_nor_octal_dtr_read(nor, addr, len, buf);
+ else
+ ret = spi_nor_read_data(nor, addr, len, buf);
if (ret == 0) {
/* We shouldn't see 0-length reads */
ret = -EIO;
--
2.30.0
On Octal DTR capable flashes like Micron Xcella the writes cannot start
or end at an odd address in Octal DTR mode. Extra 0xff bytes need to be
appended or prepended to make sure the start address and end address are
even. 0xff is used because on NOR flashes a program operation can only
flip bits from 1 to 0, not the other way round. 0 to 1 flip needs to
happen via erases.
Signed-off-by: Pratyush Yadav <[email protected]>
---
Changes in v2:
- Replace semicolon in subject with colon.
- Add a comment that ret < 0 after adjusting for extra bytes is not
possible, and add a WARN_ON() on the condition to make sure it gets
spotted quickly when some change triggers this bug.
drivers/mtd/spi-nor/core.c | 73 +++++++++++++++++++++++++++++++++++++-
1 file changed, 72 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index a696af6a1b71..d2a7e16e667d 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2023,6 +2023,72 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
return ret;
}
+/*
+ * On Octal DTR capable flashes like Micron Xcella the writes cannot start or
+ * end at an odd address in Octal DTR mode. Extra 0xff bytes need to be appended
+ * or prepended to make sure the start address and end address are even. 0xff is
+ * used because on NOR flashes a program operation can only flip bits from 1 to
+ * 0, not the other way round. 0 to 1 flip needs to happen via erases.
+ */
+static int spi_nor_octal_dtr_write(struct spi_nor *nor, loff_t to, size_t len,
+ const u8 *buf)
+{
+ u8 *tmp_buf;
+ size_t bytes_written;
+ loff_t start, end;
+ int ret;
+
+ if (IS_ALIGNED(to, 2) && IS_ALIGNED(len, 2))
+ return spi_nor_write_data(nor, to, len, buf);
+
+ tmp_buf = kmalloc(nor->page_size, GFP_KERNEL);
+ if (!tmp_buf)
+ return -ENOMEM;
+
+ memset(tmp_buf, 0xff, nor->page_size);
+
+ start = round_down(to, 2);
+ end = round_up(to + len, 2);
+
+ memcpy(tmp_buf + (to - start), buf, len);
+
+ ret = spi_nor_write_data(nor, start, end - start, tmp_buf);
+ if (ret == 0) {
+ ret = -EIO;
+ goto out;
+ }
+ if (ret < 0)
+ goto out;
+
+ /*
+ * More bytes are written than actually requested, but that number can't
+ * be reported to the calling function or it will confuse its
+ * calculations. Calculate how many of the _requested_ bytes were
+ * written.
+ */
+ bytes_written = ret;
+
+ if (to != start)
+ ret -= to - start;
+
+ /*
+ * Only account for extra bytes at the end if they were actually
+ * written. For example, if for some reason the controller could only
+ * complete a partial write then the adjustment for the extra bytes at
+ * the end is not needed.
+ */
+ if (start + bytes_written == end)
+ ret -= end - (to + len);
+
+ /* Should not be possible. */
+ if (WARN_ON(ret < 0))
+ ret = -EIO;
+
+out:
+ kfree(tmp_buf);
+ return ret;
+}
+
/*
* Write an address range to the nor chip. Data must be written in
* FLASH_PAGESIZE chunks. The address range may be any size provided
@@ -2067,7 +2133,12 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
if (ret)
goto write_err;
- ret = spi_nor_write_data(nor, addr, page_remain, buf + i);
+ if (nor->write_proto == SNOR_PROTO_8_8_8_DTR)
+ ret = spi_nor_octal_dtr_write(nor, addr, page_remain,
+ buf + i);
+ else
+ ret = spi_nor_write_data(nor, addr, page_remain,
+ buf + i);
if (ret < 0)
goto write_err;
written = ret;
--
2.30.0
Am 2021-05-31 20:17, schrieb Pratyush Yadav:
> The template ops used in spi_nor_spimem_check_pp() and
> spi_nor_spimem_check_readop() currently set the data phase to 1 byte
> long. This is problematic for 8D-8D-8D protocol where odd length data
> phase is invalid since one cycle transfers 2 bytes and odd number of
> bytes would mean half a cycle is left over. This could result in a
> controller rejecting the op as "not supported" even though it actually
> supports the protocol.
>
> Change the data length to 2 bytes in these templates. One might argue
> that this should only be done for 8D-8D-8D operations but when talking
> about these templates, there is no functional difference between one
> and
> two bytes, even in STR modes.
>
> Signed-off-by: Pratyush Yadav <[email protected]>
> ---
Reviewed-by: Michael Walle <[email protected]>
Am 2021-05-31 20:17, schrieb Pratyush Yadav:
> On Octal DTR capable flashes like Micron Xcella the writes cannot start
> or end at an odd address in Octal DTR mode. Extra 0xff bytes need to be
> appended or prepended to make sure the start address and end address
> are
> even. 0xff is used because on NOR flashes a program operation can only
> flip bits from 1 to 0, not the other way round. 0 to 1 flip needs to
> happen via erases.
>
> Signed-off-by: Pratyush Yadav <[email protected]>
Reviewed-by: Michael Walle <[email protected]>
Am 2021-05-31 20:17, schrieb Pratyush Yadav:
> The Octal DTR configuration is stored in the CFR5V register. This
> register is 1 byte wide. But 1 byte long transactions are not allowed
> in
> 8D-8D-8D mode. Since the next byte address does not contain any
> register, it is safe to write any value to it. Write a 0 to it.
>
> Signed-off-by: Pratyush Yadav <[email protected]>
> ---
Can't say much, because there is no public datasheet, is there?
But looks sane. Same for patch 3/6.
On 01/06/21 02:47PM, Michael Walle wrote:
> Am 2021-05-31 20:17, schrieb Pratyush Yadav:
> > The Octal DTR configuration is stored in the CFR5V register. This
> > register is 1 byte wide. But 1 byte long transactions are not allowed in
> > 8D-8D-8D mode. Since the next byte address does not contain any
> > register, it is safe to write any value to it. Write a 0 to it.
> >
> > Signed-off-by: Pratyush Yadav <[email protected]>
> > ---
>
> Can't say much, because there is no public datasheet, is there?
https://www.cypress.com/file/513996/download
>
> But looks sane. Same for patch 3/6.
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
On 5/31/21 9:17 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> In 8D-8D-8D mode two bytes are transferred per cycle. So an odd number
> of bytes cannot be transferred because it would leave a residual half
> cycle at the end. Consider such a transfer invalid and reject it.
>
> Signed-off-by: Pratyush Yadav <[email protected]>
> Reviewed-by: Mark Brown <[email protected]>
>
> ---
>
> Changes in v2:
> - Add Mark's R-by (spell corrected).
>
> drivers/spi/spi-mem.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index 1513553e4080..ab9eefbaa1d9 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -162,7 +162,17 @@ static bool spi_mem_check_buswidth(struct spi_mem *mem,
> bool spi_mem_dtr_supports_op(struct spi_mem *mem,
> const struct spi_mem_op *op)
> {
> - if (op->cmd.nbytes != 2)
> + if (op->cmd.buswidth == 8 && op->cmd.nbytes % 2)
!IS_ALIGNED(op->cmd.nbytes, 2)?
> + return false;
> +
> + if (op->addr.nbytes && op->addr.buswidth == 8 && op->addr.nbytes % 2)
> + return false;
> +
> + if (op->dummy.nbytes && op->dummy.buswidth == 8 && op->dummy.nbytes % 2)
> + return false;
> +
> + if (op->data.dir != SPI_MEM_NO_DATA &&
> + op->dummy.buswidth == 8 && op->data.nbytes % 2)
dummy is sent on the same buswidth as data's indeed, but I would
s/op->dummy.buswidth/op->data.buswidth for code consistency reasons.
> return false;
>
> return spi_mem_check_buswidth(mem, op);
> --
> 2.30.0
>
On 23/12/21 11:43AM, [email protected] wrote:
> On 5/31/21 9:17 PM, Pratyush Yadav wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > In 8D-8D-8D mode two bytes are transferred per cycle. So an odd number
> > of bytes cannot be transferred because it would leave a residual half
> > cycle at the end. Consider such a transfer invalid and reject it.
> >
> > Signed-off-by: Pratyush Yadav <[email protected]>
> > Reviewed-by: Mark Brown <[email protected]>
> >
> > ---
> >
> > Changes in v2:
> > - Add Mark's R-by (spell corrected).
> >
> > drivers/spi/spi-mem.c | 12 +++++++++++-
> > 1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> > index 1513553e4080..ab9eefbaa1d9 100644
> > --- a/drivers/spi/spi-mem.c
> > +++ b/drivers/spi/spi-mem.c
> > @@ -162,7 +162,17 @@ static bool spi_mem_check_buswidth(struct spi_mem *mem,
> > bool spi_mem_dtr_supports_op(struct spi_mem *mem,
> > const struct spi_mem_op *op)
> > {
> > - if (op->cmd.nbytes != 2)
> > + if (op->cmd.buswidth == 8 && op->cmd.nbytes % 2)
>
> !IS_ALIGNED(op->cmd.nbytes, 2)?
Ok.
>
> > + return false;
> > +
> > + if (op->addr.nbytes && op->addr.buswidth == 8 && op->addr.nbytes % 2)
> > + return false;
> > +
> > + if (op->dummy.nbytes && op->dummy.buswidth == 8 && op->dummy.nbytes % 2)
> > + return false;
> > +
> > + if (op->data.dir != SPI_MEM_NO_DATA &&
> > + op->dummy.buswidth == 8 && op->data.nbytes % 2)
>
> dummy is sent on the same buswidth as data's indeed, but I would
> s/op->dummy.buswidth/op->data.buswidth for code consistency reasons.
This looks like a typo. It should indeed be data.buswidth.
>
> > return false;
> >
> > return spi_mem_check_buswidth(mem, op);
> > --
> > 2.30.0
> >
>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
On 5/31/21 9:17 PM, Pratyush Yadav wrote:
Hi!
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Octal DTR capable flashes like Micron Xcella reads cannot start or
> end at an odd address in Octal DTR mode. Extra bytes need to be read at
> the start or end to make sure both the start address and length remain
> even.
>
> To avoid allocating too much extra memory, thereby putting unnecessary
> memory pressure on the system, the temporary buffer containing the extra
> padding bytes is capped at PAGE_SIZE bytes. The rest of the 2-byte
> aligned part should be read directly in the main buffer.
>
> Signed-off-by: Pratyush Yadav <[email protected]>
> Reviewed-by: Michael Walle <[email protected]>
>
> ---
>
> Changes in v2:
> - Replace semicolon in subject with colon.
> - Add a comment that ret < 0 after adjusting for extra bytes is not
> possible, and add a WARN_ON() on the condition to make sure it gets
> spotted quickly when some change triggers this bug.
> - Add Michael's R-by.
>
> drivers/mtd/spi-nor/core.c | 82 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 81 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index d521ca577884..a696af6a1b71 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1904,6 +1904,83 @@ static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
> return ERR_PTR(-ENODEV);
> }
>
> +/*
> + * On Octal DTR capable flashes like Micron Xcella reads cannot start or
> + * end at an odd address in Octal DTR mode. Extra bytes need to be read
> + * at the start or end to make sure both the start address and length
> + * remain even.
> + */
> +static int spi_nor_octal_dtr_read(struct spi_nor *nor, loff_t from, size_t len,
> + u_char *buf)
> +{
> + u_char *tmp_buf;
> + size_t tmp_len;
> + loff_t start, end;
> + int ret, bytes_read;
> +
> + if (IS_ALIGNED(from, 2) && IS_ALIGNED(len, 2))
> + return spi_nor_read_data(nor, from, len, buf);
> + else if (IS_ALIGNED(from, 2) && len > PAGE_SIZE)
> + return spi_nor_read_data(nor, from, round_down(len, PAGE_SIZE),
> + buf);
> +
> + tmp_buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
> + if (!tmp_buf)
> + return -ENOMEM;
> +
> + start = round_down(from, 2);
> + end = round_up(from + len, 2);
> +
> + /*
> + * Avoid allocating too much memory. The requested read length might be
> + * quite large. Allocating a buffer just as large (slightly bigger, in
> + * fact) would put unnecessary memory pressure on the system.
> + *
> + * For example if the read is from 3 to 1M, then this will read from 2
> + * to 4098. The reads from 4098 to 1M will then not need a temporary
> + * buffer so they can proceed as normal.
> + */
> + tmp_len = min_t(size_t, end - start, PAGE_SIZE);
> +
> + ret = spi_nor_read_data(nor, start, tmp_len, tmp_buf);
> + if (ret == 0) {
> + ret = -EIO;
> + goto out;
> + }
> + if (ret < 0)
> + goto out;
> +
> + /*
> + * More bytes are read than actually requested, but that number can't be
> + * reported to the calling function or it will confuse its calculations.
> + * Calculate how many of the _requested_ bytes were read.
> + */
> + bytes_read = ret;
> +
> + if (from != start)
> + ret -= from - start;
> +
> + /*
> + * Only account for extra bytes at the end if they were actually read.
> + * For example, if the total length was truncated because of temporary
> + * buffer size limit then the adjustment for the extra bytes at the end
> + * is not needed.
> + */
> + if (start + bytes_read == end)
> + ret -= end - (from + len);
> +
> + /* Should not be possible. */
> + if (WARN_ON(ret < 0)) {
> + ret = -EIO;
> + goto out;
> + }
then why do we keep it? What are the cases in which ret < 0?
Cheers,
ta
> +
> + memcpy(buf, tmp_buf + (from - start), ret);
> +out:
> + kfree(tmp_buf);
> + return ret;
> +}
> +
> static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
> size_t *retlen, u_char *buf)
> {
> @@ -1921,7 +1998,10 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
>
> addr = spi_nor_convert_addr(nor, addr);
>
> - ret = spi_nor_read_data(nor, addr, len, buf);
> + if (nor->read_proto == SNOR_PROTO_8_8_8_DTR)
> + ret = spi_nor_octal_dtr_read(nor, addr, len, buf);
> + else
> + ret = spi_nor_read_data(nor, addr, len, buf);
> if (ret == 0) {
> /* We shouldn't see 0-length reads */
> ret = -EIO;
> --
> 2.30.0
>
On 5/31/21 9:17 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Octal DTR capable flashes like Micron Xcella the writes cannot start
> or end at an odd address in Octal DTR mode. Extra 0xff bytes need to be
> appended or prepended to make sure the start address and end address are
> even. 0xff is used because on NOR flashes a program operation can only
> flip bits from 1 to 0, not the other way round. 0 to 1 flip needs to
> happen via erases.
>
> Signed-off-by: Pratyush Yadav <[email protected]>
>
> ---
>
> Changes in v2:
> - Replace semicolon in subject with colon.
> - Add a comment that ret < 0 after adjusting for extra bytes is not
> possible, and add a WARN_ON() on the condition to make sure it gets
> spotted quickly when some change triggers this bug.
>
> drivers/mtd/spi-nor/core.c | 73 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 72 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index a696af6a1b71..d2a7e16e667d 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2023,6 +2023,72 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
> return ret;
> }
>
> +/*
let's add kernel-doc comments for new methods.
> + * On Octal DTR capable flashes like Micron Xcella the writes cannot start or
strip "the" from "the writes". But I think I would get rid of the Micron Xcella example,
we're using these methods for all the 8D-8D-8D flashes. You can mention Micron in the
commit message if you want, but we shouldn't mention manufacturers in the core.
> + * end at an odd address in Octal DTR mode. Extra 0xff bytes need to be appended
> + * or prepended to make sure the start address and end address are even. 0xff is
> + * used because on NOR flashes a program operation can only flip bits from 1 to
> + * 0, not the other way round. 0 to 1 flip needs to happen via erases.
> + */
> +static int spi_nor_octal_dtr_write(struct spi_nor *nor, loff_t to, size_t len,
> + const u8 *buf)
> +{
> + u8 *tmp_buf;
> + size_t bytes_written;
> + loff_t start, end;
> + int ret;
> +
> + if (IS_ALIGNED(to, 2) && IS_ALIGNED(len, 2))
> + return spi_nor_write_data(nor, to, len, buf);
> +
> + tmp_buf = kmalloc(nor->page_size, GFP_KERNEL);> + if (!tmp_buf)
> + return -ENOMEM;
> +
> + memset(tmp_buf, 0xff, nor->page_size);
> +
> + start = round_down(to, 2);
> + end = round_up(to + len, 2);
> +
> + memcpy(tmp_buf + (to - start), buf, len);
> +
> + ret = spi_nor_write_data(nor, start, end - start, tmp_buf);
> + if (ret == 0) {
> + ret = -EIO;
> + goto out;
> + }
> + if (ret < 0)
> + goto out;
> +
> + /*
> + * More bytes are written than actually requested, but that number can't
> + * be reported to the calling function or it will confuse its
> + * calculations. Calculate how many of the _requested_ bytes were
> + * written.
> + */
> + bytes_written = ret;
> +
> + if (to != start)
> + ret -= to - start;
> +
> + /*
> + * Only account for extra bytes at the end if they were actually
> + * written. For example, if for some reason the controller could only
> + * complete a partial write then the adjustment for the extra bytes at
> + * the end is not needed.
> + */
> + if (start + bytes_written == end)
> + ret -= end - (to + len);
> +
> + /* Should not be possible. */
> + if (WARN_ON(ret < 0))
> + ret = -EIO;
Is this really needed? Also, I think I would squash patch 5 and 6,
it's the same idea, and reads and writes are tied together.
Looks good!
> +
> +out:
> + kfree(tmp_buf);
> + return ret;
> +}
> +
> /*
> * Write an address range to the nor chip. Data must be written in
> * FLASH_PAGESIZE chunks. The address range may be any size provided
> @@ -2067,7 +2133,12 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
> if (ret)
> goto write_err;
>
> - ret = spi_nor_write_data(nor, addr, page_remain, buf + i);
> + if (nor->write_proto == SNOR_PROTO_8_8_8_DTR)
> + ret = spi_nor_octal_dtr_write(nor, addr, page_remain,
> + buf + i);
> + else
> + ret = spi_nor_write_data(nor, addr, page_remain,
> + buf + i);
> if (ret < 0)
> goto write_err;
> written = ret;
> --
> 2.30.0
>
On 5/31/21 9:17 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> The Octal DTR configuration is stored in the CFR5V register. This
> register is 1 byte wide. But 1 byte long transactions are not allowed in
> 8D-8D-8D mode. Since the next byte address does not contain any
> register, it is safe to write any value to it. Write a 0 to it.
>
> Signed-off-by: Pratyush Yadav <[email protected]>
> ---
>
> (no changes since v1)
>
> drivers/mtd/spi-nor/spansion.c | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index ee82dcd75310..e6bf5c9eee6a 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -65,10 +65,18 @@ static int spi_nor_cypress_octal_dtr_enable(struct spi_nor *nor, bool enable)
> if (ret)
> return ret;
>
> - if (enable)
> - *buf = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN;
> - else
> - *buf = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS;
> + if (enable) {
> + buf[0] = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN;
> + } else {
> + /*
> + * The register is 1-byte wide, but 1-byte transactions are not
> + * allowed in 8D-8D-8D mode. Since there is no register at the
> + * next location, just initialize the value to 0 and let the
> + * transaction go on.
> + */
> + buf[0] = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS;
> + buf[1] = 0;
how about writing 0xff instead?
> + }
>
> op = (struct spi_mem_op)
> SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_ANY_REG, 1),
> @@ -76,7 +84,7 @@ static int spi_nor_cypress_octal_dtr_enable(struct spi_nor *nor, bool enable)
> SPINOR_REG_CYPRESS_CFR5V,
> 1),
> SPI_MEM_OP_NO_DUMMY,
> - SPI_MEM_OP_DATA_OUT(1, buf, 1));
> + SPI_MEM_OP_DATA_OUT(enable ? 1 : 2, buf, 1));
>
> if (!enable)
> spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
> --
> 2.30.0
>
On 12/23/21 3:06 PM, Tudor Ambarus wrote:
> On 5/31/21 9:17 PM, Pratyush Yadav wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> The Octal DTR configuration is stored in the CFR5V register. This
>> register is 1 byte wide. But 1 byte long transactions are not allowed in
>> 8D-8D-8D mode. Since the next byte address does not contain any
>> register, it is safe to write any value to it. Write a 0 to it.
>>
>> Signed-off-by: Pratyush Yadav <[email protected]>
>> ---
>>
>> (no changes since v1)
>>
>> drivers/mtd/spi-nor/spansion.c | 18 +++++++++++++-----
>> 1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>> index ee82dcd75310..e6bf5c9eee6a 100644
>> --- a/drivers/mtd/spi-nor/spansion.c
>> +++ b/drivers/mtd/spi-nor/spansion.c
>> @@ -65,10 +65,18 @@ static int spi_nor_cypress_octal_dtr_enable(struct spi_nor *nor, bool enable)
>> if (ret)
>> return ret;
>>
>> - if (enable)
>> - *buf = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN;
>> - else
>> - *buf = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS;
>> + if (enable) {
>> + buf[0] = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN;
>> + } else {
>> + /*
>> + * The register is 1-byte wide, but 1-byte transactions are not
>> + * allowed in 8D-8D-8D mode. Since there is no register at the
>> + * next location, just initialize the value to 0 and let the
>> + * transaction go on.
>> + */
>> + buf[0] = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS;
>> + buf[1] = 0;
>
> how about writing 0xff instead?
patches 1, 2 and 3 look fine, except for this comment. Would you resend them, or you want
me to do the change locally when applying? Send me an updated comment if so.
>
>> + }
>>
>> op = (struct spi_mem_op)
>> SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_ANY_REG, 1),
>> @@ -76,7 +84,7 @@ static int spi_nor_cypress_octal_dtr_enable(struct spi_nor *nor, bool enable)
>> SPINOR_REG_CYPRESS_CFR5V,
>> 1),
>> SPI_MEM_OP_NO_DUMMY,
>> - SPI_MEM_OP_DATA_OUT(1, buf, 1));
>> + SPI_MEM_OP_DATA_OUT(enable ? 1 : 2, buf, 1));
>>
>> if (!enable)
>> spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
>> --
>> 2.30.0
>>
>
On 12/23/21 3:06 PM, [email protected] wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 5/31/21 9:17 PM, Pratyush Yadav wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> The Octal DTR configuration is stored in the CFR5V register. This
>> register is 1 byte wide. But 1 byte long transactions are not allowed in
>> 8D-8D-8D mode. Since the next byte address does not contain any
>> register, it is safe to write any value to it. Write a 0 to it.
>>
>> Signed-off-by: Pratyush Yadav <[email protected]>
>> ---
>>
>> (no changes since v1)
>>
>> drivers/mtd/spi-nor/spansion.c | 18 +++++++++++++-----
>> 1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>> index ee82dcd75310..e6bf5c9eee6a 100644
>> --- a/drivers/mtd/spi-nor/spansion.c
>> +++ b/drivers/mtd/spi-nor/spansion.c
>> @@ -65,10 +65,18 @@ static int spi_nor_cypress_octal_dtr_enable(struct spi_nor *nor, bool enable)
>> if (ret)
>> return ret;
>>
>> - if (enable)
>> - *buf = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN;
>> - else
>> - *buf = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS;
>> + if (enable) {
>> + buf[0] = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN;
>> + } else {
>> + /*
>> + * The register is 1-byte wide, but 1-byte transactions are not
>> + * allowed in 8D-8D-8D mode. Since there is no register at the
>> + * next location, just initialize the value to 0 and let the
>> + * transaction go on.
>> + */
>> + buf[0] = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS;
>> + buf[1] = 0;
>
> how about writing 0xff instead?
it doesn't matter, it's a register. Will apply first 3.
>
>> + }
>>
>> op = (struct spi_mem_op)
>> SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_ANY_REG, 1),
>> @@ -76,7 +84,7 @@ static int spi_nor_cypress_octal_dtr_enable(struct spi_nor *nor, bool enable)
>> SPINOR_REG_CYPRESS_CFR5V,
>> 1),
>> SPI_MEM_OP_NO_DUMMY,
>> - SPI_MEM_OP_DATA_OUT(1, buf, 1));
>> + SPI_MEM_OP_DATA_OUT(enable ? 1 : 2, buf, 1));
>>
>> if (!enable)
>> spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
>> --
>> 2.30.0
>>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
On Mon, 31 May 2021 23:47:51 +0530, Pratyush Yadav wrote:
> On Octal DTR flashes like Micron Xcella or Cypress S28 family, reads or
> writes cannot start at an odd address in 8D-8D-8D mode. Neither can they
> be odd bytes long. Upper layers like filesystems don't know what mode
> the flash is in, and hence don't know the read/write address or length
> limitations. They might issue reads or writes that leave the flash in an
> error state. In fact, using UBIFS on top of the flash was how I first
> noticed this problem.
>
> [...]
Applied to spi-nor/next, thanks!
[1/6] mtd: spi-nor: core: use 2 data bytes for template ops
https://git.kernel.org/mtd/c/0d051a49829a
[2/6] mtd: spi-nor: spansion: write 2 bytes when disabling Octal DTR mode
https://git.kernel.org/mtd/c/63017068a6d9
[3/6] mtd: spi-nor: micron-st: write 2 bytes when disabling Octal DTR mode
https://git.kernel.org/mtd/c/9de3cb1cc95b
Best regards,
--
Tudor Ambarus <[email protected]>