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