[PATCH 0/6] Avoid odd length/address read/writes in 8D-8D-8D mode.

[PATCH 0/6] Avoid odd length/address read/writes in 8D-8D-8D mode.

[Pratyush Yadav]

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.


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      | 157 +++++++++++++++++++++++++++++++-
 drivers/mtd/spi-nor/micron-st.c |  22 ++++-
 drivers/mtd/spi-nor/spansion.c  |  18 +++-
 drivers/spi/spi-mem.c           |  12 ++-
 4 files changed, 194 insertions(+), 15 deletions(-)

-- 
2.30.0

[PATCH 1/6] mtd: spi-nor: core: use 2 data bytes for template ops

[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 <p.yadav@ti.com>
---

 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 bd2c7717eb10..5cc206b8bbf3 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

[PATCH 2/6] mtd: spi-nor: spansion: write 2 bytes when disabling Octal DTR mode

[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 <p.yadav@ti.com>
---

 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

[PATCH 3/6] mtd: spi-nor: micron-st: write 2 bytes when disabling Octal DTR mode

[Pratyush Yadav]

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 <p.yadav@ti.com>
---

 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

[PATCH 4/6] spi: spi-mem: reject partial cycle transfers in 8D-8D-8D mode

[Pratyush Yadav]

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 <p.yadav@ti.com>

---
This patch 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.

 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

Re: [PATCH 4/6] spi: spi-mem: reject partial cycle transfers in 8D-8D-8D mode

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 762 bytes --]

On Fri, May 07, 2021 at 12:48:27AM +0530, Pratyush Yadav wrote:
> 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 <p.yadav@ti.com>
> 
> ---
> This patch 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.

It seems like this should probably even go in as a fix even if nothing
is broken with mainline right now, it's the sort of thing some out of
tree patch could easily trigger.  Unless anyone objects I'll do that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/6] spi: spi-mem: reject partial cycle transfers in 8D-8D-8D mode

[Pratyush Yadav]

On 07/05/21 01:55PM, Mark Brown wrote:
> On Fri, May 07, 2021 at 12:48:27AM +0530, Pratyush Yadav wrote:
> > 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 <p.yadav@ti.com>
> > 
> > ---
> > This patch 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.
> 
> It seems like this should probably even go in as a fix even if nothing
> is broken with mainline right now, it's the sort of thing some out of
> tree patch could easily trigger.  Unless anyone objects I'll do that.

If it does get backported to stable branches, patches 1-3 would have to 
go in _before_ this one. Otherwise it will break the two 8D-8D-8D 
flashes: Micron MT35XU512ABA and Cypress S28HS512T. Without patch 1 
8D-8D-8D mode will not be selected correctly on these two flashes. The 
supports_op() will reject it because of 1 data byte. This is a 
relatively safe patch for backporting to a stable branch.

Patches 2 and 3 are a slightly different matter. They add an extra 
register write. But most controllers I've come across don't support 
1-byte writes in 8D mode. It is likely that they are sending 
bogus/undefined values in the second byte and deasserting CS only after 
the cycle is done. So they should _in theory_ change undefined behaviour 
to defined behaviour.

Still, they introduce an extra register write. I'm not sure how 
risk-tolerant you want to be for stable backports. I will leave the 
judgement to you or Tudor or Vignesh.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH 4/6] spi: spi-mem: reject partial cycle transfers in 8D-8D-8D mode

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 723 bytes --]

On Fri, May 07, 2021 at 07:26:33PM +0530, Pratyush Yadav wrote:

> Patches 2 and 3 are a slightly different matter. They add an extra 
> register write. But most controllers I've come across don't support 
> 1-byte writes in 8D mode. It is likely that they are sending 
> bogus/undefined values in the second byte and deasserting CS only after 
> the cycle is done. So they should _in theory_ change undefined behaviour 
> to defined behaviour.

> Still, they introduce an extra register write. I'm not sure how 
> risk-tolerant you want to be for stable backports. I will leave the 
> judgement to you or Tudor or Vignesh.

Ah, given that if nobody's seeing any issues I'd probably just hold off
there TBH.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/6] spi: spi-mem: reject partial cycle transfers in 8D-8D-8D mode

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 322 bytes --]

On Fri, May 07, 2021 at 12:48:27AM +0530, Pratyush Yadav wrote:
> 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.

Reviwed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/6] spi: spi-mem: reject partial cycle transfers in 8D-8D-8D mode

[Pratyush Yadav]

On 07/05/21 04:48PM, Mark Brown wrote:
> On Fri, May 07, 2021 at 12:48:27AM +0530, Pratyush Yadav wrote:
> > 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.
> 
> Reviwed-by: Mark Brown <broonie@kernel.org>

Thanks. BTW, s/Reviwed/Reviewed/.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH 5/6] mtd: spi-nor: core; avoid odd length/address reads on 8D-8D-8D mode

[Pratyush Yadav]

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 <p.yadav@ti.com>
---

 drivers/mtd/spi-nor/core.c | 81 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 80 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 5cc206b8bbf3..3d66cc34af4d 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1904,6 +1904,82 @@ 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);
+
+	if (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 +1997,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

Re: [PATCH 5/6] mtd: spi-nor: core; avoid odd length/address reads on 8D-8D-8D mode

[Michael Walle]

Am 2021-05-06 21:18, schrieb Pratyush Yadav:
> 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 <p.yadav@ti.com>
> ---
> 
>  drivers/mtd/spi-nor/core.c | 81 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 80 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 5cc206b8bbf3..3d66cc34af4d 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1904,6 +1904,82 @@ 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);
> +
> +	if (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 +1997,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;

Reviewed-by: Michael Walle <michael@walle.cc>

I wonder how much performance is lost if this would just split
one transfer into up to three ones: 2 byte, size - 2, 2 bytes.

-michael

Re: [PATCH 5/6] mtd: spi-nor: core; avoid odd length/address reads on 8D-8D-8D mode

[Pratyush Yadav]

On 07/05/21 05:51PM, Michael Walle wrote:
> Am 2021-05-06 21:18, schrieb Pratyush Yadav:
> > 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 <p.yadav@ti.com>
> > ---
> > 
> >  drivers/mtd/spi-nor/core.c | 81 +++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 80 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index 5cc206b8bbf3..3d66cc34af4d 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -1904,6 +1904,82 @@ 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);
> > +
> > +	if (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 +1997,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;
> 
> Reviewed-by: Michael Walle <michael@walle.cc>

Thanks.

> 
> I wonder how much performance is lost if this would just split
> one transfer into up to three ones: 2 byte, size - 2, 2 bytes.

This case is not really possible since it would try to read PAGE_SIZE 
whenever it can. But there is a situation possible where one transfer is 
split into three. It would look something like: 4096 bytes, size - 4096 
bytes, 2 bytes.

I am trying to find a balance between minimizing number of reads while 
keeping the size of the temporary buffer to a reasonable limit. This is 
the best I could come up with. It optimizes for smaller transfers so 
while the absolute amount of overhead remains roughly the same, the 
ratio of it relative to read size is smaller.

You can optimize for read performance if you are willing to waste memory 
by simple allocating a size + 2 bytes long buffer. Then the read can 
proceed in one transaction. But IMO memory is much more important 
compared to read throughput.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH 5/6] mtd: spi-nor: core; avoid odd length/address reads on 8D-8D-8D mode

[Michael Walle]

Am 2021-05-07 20:04, schrieb Pratyush Yadav:
> On 07/05/21 05:51PM, Michael Walle wrote:
>> Am 2021-05-06 21:18, schrieb Pratyush Yadav:
>> > 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 <p.yadav@ti.com>
>> > ---
>> >
>> >  drivers/mtd/spi-nor/core.c | 81 +++++++++++++++++++++++++++++++++++++-
>> >  1 file changed, 80 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> > index 5cc206b8bbf3..3d66cc34af4d 100644
>> > --- a/drivers/mtd/spi-nor/core.c
>> > +++ b/drivers/mtd/spi-nor/core.c
>> > @@ -1904,6 +1904,82 @@ 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);
>> > +
>> > +	if (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 +1997,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;
>> 
>> Reviewed-by: Michael Walle <michael@walle.cc>
> 
> Thanks.
> 
>> 
>> I wonder how much performance is lost if this would just split
>> one transfer into up to three ones: 2 byte, size - 2, 2 bytes.
> 
> This case is not really possible since it would try to read PAGE_SIZE
> whenever it can. But there is a situation possible where one transfer 
> is
> split into three. It would look something like: 4096 bytes, size - 4096
> bytes, 2 bytes.

Ah no, I wasn't talking about your implementation, but just having a 
naive
one where you don't move around up to PAGE_SIZE of data but just read
2 bytes in the beginning (if unaligned) and 2 bytes at the end (if 
unaligned)
and reading the part in between just as usual because its then aligend.

> I am trying to find a balance between minimizing number of reads while
> keeping the size of the temporary buffer to a reasonable limit. This is
> the best I could come up with. It optimizes for smaller transfers so
> while the absolute amount of overhead remains roughly the same, the
> ratio of it relative to read size is smaller.

Yes, with this you will have that memcpy() and one transfer for 
transfers
up to PAGE_SIZE; the "naive" one above would have up to three depending 
on
the aligment.

> You can optimize for read performance if you are willing to waste 
> memory
> by simple allocating a size + 2 bytes long buffer. Then the read can
> proceed in one transaction. But IMO memory is much more important
> compared to read throughput.

-michael

Re: [PATCH 5/6] mtd: spi-nor: core; avoid odd length/address reads on 8D-8D-8D mode

[Pratyush Yadav]

On 07/05/21 08:14PM, Michael Walle wrote:
> Am 2021-05-07 20:04, schrieb Pratyush Yadav:
> > On 07/05/21 05:51PM, Michael Walle wrote:
> > > Am 2021-05-06 21:18, schrieb Pratyush Yadav:
> > > > 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 <p.yadav@ti.com>
> > > > ---
> > > >
> > > >  drivers/mtd/spi-nor/core.c | 81 +++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 80 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > > > index 5cc206b8bbf3..3d66cc34af4d 100644
> > > > --- a/drivers/mtd/spi-nor/core.c
> > > > +++ b/drivers/mtd/spi-nor/core.c
> > > > @@ -1904,6 +1904,82 @@ 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);
> > > > +
> > > > +	if (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 +1997,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;
> > > 
> > > Reviewed-by: Michael Walle <michael@walle.cc>
> > 
> > Thanks.
> > 
> > > 
> > > I wonder how much performance is lost if this would just split
> > > one transfer into up to three ones: 2 byte, size - 2, 2 bytes.
> > 
> > This case is not really possible since it would try to read PAGE_SIZE
> > whenever it can. But there is a situation possible where one transfer is
> > split into three. It would look something like: 4096 bytes, size - 4096
> > bytes, 2 bytes.
> 
> Ah no, I wasn't talking about your implementation, but just having a naive
> one where you don't move around up to PAGE_SIZE of data but just read
> 2 bytes in the beginning (if unaligned) and 2 bytes at the end (if
> unaligned)
> and reading the part in between just as usual because its then aligend.
> 
> > I am trying to find a balance between minimizing number of reads while
> > keeping the size of the temporary buffer to a reasonable limit. This is
> > the best I could come up with. It optimizes for smaller transfers so
> > while the absolute amount of overhead remains roughly the same, the
> > ratio of it relative to read size is smaller.
> 
> Yes, with this you will have that memcpy() and one transfer for transfers
> up to PAGE_SIZE; the "naive" one above would have up to three depending on
> the aligment.

Right. Smaller transfers lose much more performance to the overhead than 
the larger ones do. So I think the optimization is worth the extra code 
complexity.

> 
> > You can optimize for read performance if you are willing to waste memory
> > by simple allocating a size + 2 bytes long buffer. Then the read can
> > proceed in one transaction. But IMO memory is much more important
> > compared to read throughput.
> 
> -michael

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH 6/6] mtd: spi-nor: core; avoid odd length/address writes in 8D-8D-8D mode

[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 <p.yadav@ti.com>

---

 drivers/mtd/spi-nor/core.c | 72 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 71 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 3d66cc34af4d..265d8b25fc7f 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2022,6 +2022,71 @@ 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);
+
+	if (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
@@ -2066,7 +2131,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

Re: [PATCH 6/6] mtd: spi-nor: core; avoid odd length/address writes in 8D-8D-8D mode

[Michael Walle]

Am 2021-05-06 21:18, 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 <p.yadav@ti.com>
> 
> ---
> 
>  drivers/mtd/spi-nor/core.c | 72 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 71 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 3d66cc34af4d..265d8b25fc7f 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2022,6 +2022,71 @@ 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);

This could be replaced by just setting the first and the
last byte to 0xff. But this might be easier to read. I am
fine with both.

> +
> +	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;
> +	}
else if ? I've missed this in the other patch.

> +	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);
> +
> +	if (ret < 0)
> +		ret = -EIO;

can this happen?

> +
> +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
> @@ -2066,7 +2131,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;

-michael

Re: [PATCH 6/6] mtd: spi-nor: core; avoid odd length/address writes in 8D-8D-8D mode

[Pratyush Yadav]

On 07/05/21 05:56PM, Michael Walle wrote:
> Am 2021-05-06 21:18, 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 <p.yadav@ti.com>
> > 
> > ---
> > 
> >  drivers/mtd/spi-nor/core.c | 72 +++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 71 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index 3d66cc34af4d..265d8b25fc7f 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -2022,6 +2022,71 @@ 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);
> 
> This could be replaced by just setting the first and the
> last byte to 0xff. But this might be easier to read. I am
> fine with both.

First, yes. Not the last. The buffer is allocated to nor->page_size for 
simplicity but the write could be smaller than nor->page_size. So you'd 
need to calculate the position of the other 0xff byte. It is much 
simpler to just initialize the whole buffer. It will be around 256 or 
512 bytes so not a big overhead.

> 
> > +
> > +	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;
> > +	}
> else if ? I've missed this in the other patch.

Following the style used in spi_nor_read(). Anyway, I've seen 
conflicting advice on which style to be used. Some people don't like 
else if when the if ends in a return since it is effectively an else if. 
Others like it the other way round. Dunno...

> 
> > +	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);
> > +
> > +	if (ret < 0)
> > +		ret = -EIO;
> 
> can this happen?

I don't think so. IIRC this is left over from when I tried a different 
approach. Maybe I should change it to WARN_ON() to catch future 
programming errors? Though I don't mind if we drop it entirely.

> 
> > +
> > +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
> > @@ -2066,7 +2131,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;
> 
> -michael

Thanks for reviewing.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH 0/6] Avoid odd length/address read/writes in 8D-8D-8D mode.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 387 bytes --]

On Fri, May 07, 2021 at 12:48:23AM +0530, Pratyush Yadav wrote:

> 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.

It probably makes sense to apply these to the MTD tree and then send me
a pull request to avoid any future conflicts with SPI.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]