| Message ID | 20190801162229.28897-2-vigneshr@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | Merge m25p80 into spi-nor | expand |
On 08/01/2019 07:22 PM, Vignesh Raghavendra wrote: > External E-Mail > > > spi-mem layer expects all buffers passed to it to be DMA'able. But > spi-nor layer mostly allocates buffers on stack for reading/writing to > registers and therefore are not DMA'able. Introduce bounce buffer to be > used to read/write to registers. This ensures that buffer passed to > spi-mem layer during register read/writes is DMA'able. With this change > nor->cmd-buf is no longer used, so drop it. > > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> > Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com> > --- > > v4: > Avoid memcpy during READID > > v3: new patch > > drivers/mtd/spi-nor/spi-nor.c | 70 ++++++++++++++++++++--------------- > include/linux/mtd/spi-nor.h | 7 +++- > 2 files changed, 45 insertions(+), 32 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 03cc788511d5..e02376e1127b 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c cut > /** > @@ -1404,9 +1401,11 @@ static int write_sr_cr(struct spi_nor *nor, u8 *sr_cr) > { > int ret; > > + memcpy(nor->bouncebuf, sr_cr, 2); I'm thinking out loud. This can be avoided by forcing all the callers to use nor->bouncebuf. That would result in a: static int write_sr(struct spi_nor *nor, size_t len) write_sr_cr() can be removed. Memcopying 2 bytes is a small price to pay, we can keep things as they are, to not be too invasive. But if you think that this idea is worth it, tell. > + > write_enable(nor); > > - ret = nor->write_reg(nor, SPINOR_OP_WRSR, sr_cr, 2); > + ret = nor->write_reg(nor, SPINOR_OP_WRSR, nor->bouncebuf, 2); > if (ret < 0) { > dev_err(nor->dev, > "error while writing configuration register\n"); cut > @@ -2177,9 +2176,10 @@ static const struct flash_info spi_nor_ids[] = { > static const struct flash_info *spi_nor_read_id(struct spi_nor *nor) > { > int tmp; > - u8 id[SPI_NOR_MAX_ID_LEN]; > + u8 *id; > const struct flash_info *info; > > + id = nor->bouncebuf; nit: do init at declaration. Also, you missed a place in which you can use the bouncebuf, search by "read_reg(": ret = nor->read_reg(nor, SPINOR_OP_XRDSR, &val, 1); Cheers, ta
On 05/08/19 2:36 PM, Tudor.Ambarus@microchip.com wrote: > > > On 08/01/2019 07:22 PM, Vignesh Raghavendra wrote: >> External E-Mail >> >> >> spi-mem layer expects all buffers passed to it to be DMA'able. But >> spi-nor layer mostly allocates buffers on stack for reading/writing to >> registers and therefore are not DMA'able. Introduce bounce buffer to be >> used to read/write to registers. This ensures that buffer passed to >> spi-mem layer during register read/writes is DMA'able. With this change >> nor->cmd-buf is no longer used, so drop it. >> >> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> >> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com> >> --- >> >> v4: >> Avoid memcpy during READID >> >> v3: new patch >> >> drivers/mtd/spi-nor/spi-nor.c | 70 ++++++++++++++++++++--------------- >> include/linux/mtd/spi-nor.h | 7 +++- >> 2 files changed, 45 insertions(+), 32 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >> index 03cc788511d5..e02376e1127b 100644 >> --- a/drivers/mtd/spi-nor/spi-nor.c >> +++ b/drivers/mtd/spi-nor/spi-nor.c > > cut > >> /** >> @@ -1404,9 +1401,11 @@ static int write_sr_cr(struct spi_nor *nor, u8 *sr_cr) >> { >> int ret; >> >> + memcpy(nor->bouncebuf, sr_cr, 2); > > I'm thinking out loud. This can be avoided by forcing all the callers to use > nor->bouncebuf. That would result in a: > I can make this change and make all callers use nor->bouncebuf in next version. > static int write_sr(struct spi_nor *nor, size_t len) > > write_sr_cr() can be removed. Memcopying 2 bytes is a small price to pay, we can > keep things as they are, to not be too invasive. But if you think that this idea > is worth it, tell. > Sounds good to me. But replacing write_sr_cr() with above defintion of write_sr() should be a patch IMO> >> + >> write_enable(nor); >> >> - ret = nor->write_reg(nor, SPINOR_OP_WRSR, sr_cr, 2); >> + ret = nor->write_reg(nor, SPINOR_OP_WRSR, nor->bouncebuf, 2); >> if (ret < 0) { >> dev_err(nor->dev, >> "error while writing configuration register\n"); > > cut > >> @@ -2177,9 +2176,10 @@ static const struct flash_info spi_nor_ids[] = { >> static const struct flash_info *spi_nor_read_id(struct spi_nor *nor) >> { >> int tmp; >> - u8 id[SPI_NOR_MAX_ID_LEN]; >> + u8 *id; >> const struct flash_info *info; >> >> + id = nor->bouncebuf; > > nit: do init at declaration. > Ok. > Also, you missed a place in which you can use the bouncebuf, search by "read_reg(": > ret = nor->read_reg(nor, SPINOR_OP_XRDSR, &val, 1); > Indeed, will fix in next version! > Cheers, > ta >
On 08/05/2019 01:38 PM, Vignesh Raghavendra wrote: > External E-Mail > > > > On 05/08/19 2:36 PM, Tudor.Ambarus@microchip.com wrote: >> >> >> On 08/01/2019 07:22 PM, Vignesh Raghavendra wrote: >>> External E-Mail >>> >>> >>> spi-mem layer expects all buffers passed to it to be DMA'able. But >>> spi-nor layer mostly allocates buffers on stack for reading/writing to >>> registers and therefore are not DMA'able. Introduce bounce buffer to be >>> used to read/write to registers. This ensures that buffer passed to >>> spi-mem layer during register read/writes is DMA'able. With this change >>> nor->cmd-buf is no longer used, so drop it. >>> >>> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> >>> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com> >>> --- >>> >>> v4: >>> Avoid memcpy during READID >>> >>> v3: new patch >>> >>> drivers/mtd/spi-nor/spi-nor.c | 70 ++++++++++++++++++++--------------- >>> include/linux/mtd/spi-nor.h | 7 +++- >>> 2 files changed, 45 insertions(+), 32 deletions(-) >>> >>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >>> index 03cc788511d5..e02376e1127b 100644 >>> --- a/drivers/mtd/spi-nor/spi-nor.c >>> +++ b/drivers/mtd/spi-nor/spi-nor.c >> >> cut >> >>> /** >>> @@ -1404,9 +1401,11 @@ static int write_sr_cr(struct spi_nor *nor, u8 *sr_cr) >>> { >>> int ret; >>> >>> + memcpy(nor->bouncebuf, sr_cr, 2); >> >> I'm thinking out loud. This can be avoided by forcing all the callers to use >> nor->bouncebuf. That would result in a: >> > > I can make this change and make all callers use nor->bouncebuf in next > version. > should be ok, it will not change the flow logic. >> static int write_sr(struct spi_nor *nor, size_t len) >> >> write_sr_cr() can be removed. Memcopying 2 bytes is a small price to pay, we can >> keep things as they are, to not be too invasive. But if you think that this idea >> is worth it, tell. >> > > Sounds good to me. But replacing write_sr_cr() with above defintion of > write_sr() should be a patch IMO> I'll do a patch after we finish to integrate the big fat changes. Thanks, ta
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index 03cc788511d5..e02376e1127b 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -296,15 +296,14 @@ struct flash_info { static int read_sr(struct spi_nor *nor) { int ret; - u8 val; - ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val, 1); + ret = nor->read_reg(nor, SPINOR_OP_RDSR, nor->bouncebuf, 1); if (ret < 0) { pr_err("error %d reading SR\n", (int) ret); return ret; } - return val; + return nor->bouncebuf[0]; } /* @@ -315,15 +314,14 @@ static int read_sr(struct spi_nor *nor) static int read_fsr(struct spi_nor *nor) { int ret; - u8 val; - ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val, 1); + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, nor->bouncebuf, 1); if (ret < 0) { pr_err("error %d reading FSR\n", ret); return ret; } - return val; + return nor->bouncebuf[0]; } /* @@ -334,15 +332,14 @@ static int read_fsr(struct spi_nor *nor) static int read_cr(struct spi_nor *nor) { int ret; - u8 val; - ret = nor->read_reg(nor, SPINOR_OP_RDCR, &val, 1); + ret = nor->read_reg(nor, SPINOR_OP_RDCR, nor->bouncebuf, 1); if (ret < 0) { dev_err(nor->dev, "error %d reading CR\n", ret); return ret; } - return val; + return nor->bouncebuf[0]; } /* @@ -351,8 +348,8 @@ static int read_cr(struct spi_nor *nor) */ static int write_sr(struct spi_nor *nor, u8 val) { - nor->cmd_buf[0] = val; - return nor->write_reg(nor, SPINOR_OP_WRSR, nor->cmd_buf, 1); + nor->bouncebuf[0] = val; + return nor->write_reg(nor, SPINOR_OP_WRSR, nor->bouncebuf, 1); } /* @@ -500,31 +497,31 @@ static int set_4byte(struct spi_nor *nor, bool enable) * We must clear the register to enable normal behavior. */ write_enable(nor); - nor->cmd_buf[0] = 0; - nor->write_reg(nor, SPINOR_OP_WREAR, nor->cmd_buf, 1); + nor->bouncebuf[0] = 0; + nor->write_reg(nor, SPINOR_OP_WREAR, + nor->bouncebuf, 1); write_disable(nor); } return status; default: /* Spansion style */ - nor->cmd_buf[0] = enable << 7; - return nor->write_reg(nor, SPINOR_OP_BRWR, nor->cmd_buf, 1); + nor->bouncebuf[0] = enable << 7; + return nor->write_reg(nor, SPINOR_OP_BRWR, nor->bouncebuf, 1); } } static int s3an_sr_ready(struct spi_nor *nor) { int ret; - u8 val; - ret = nor->read_reg(nor, SPINOR_OP_XRDSR, &val, 1); + ret = nor->read_reg(nor, SPINOR_OP_XRDSR, nor->bouncebuf, 1); if (ret < 0) { dev_err(nor->dev, "error %d reading XRDSR\n", (int) ret); return ret; } - return !!(val & XSR_RDY); + return !!(nor->bouncebuf[0] & XSR_RDY); } static int spi_nor_sr_ready(struct spi_nor *nor) @@ -683,7 +680,6 @@ static loff_t spi_nor_s3an_addr_convert(struct spi_nor *nor, unsigned int addr) */ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr) { - u8 buf[SPI_NOR_MAX_ADDR_WIDTH]; int i; if (nor->flags & SNOR_F_S3AN_ADDR_DEFAULT) @@ -697,11 +693,12 @@ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr) * control */ for (i = nor->addr_width - 1; i >= 0; i--) { - buf[i] = addr & 0xff; + nor->bouncebuf[i] = addr & 0xff; addr >>= 8; } - return nor->write_reg(nor, nor->erase_opcode, buf, nor->addr_width); + return nor->write_reg(nor, nor->erase_opcode, nor->bouncebuf, + nor->addr_width); } /** @@ -1404,9 +1401,11 @@ static int write_sr_cr(struct spi_nor *nor, u8 *sr_cr) { int ret; + memcpy(nor->bouncebuf, sr_cr, 2); + write_enable(nor); - ret = nor->write_reg(nor, SPINOR_OP_WRSR, sr_cr, 2); + ret = nor->write_reg(nor, SPINOR_OP_WRSR, nor->bouncebuf, 2); if (ret < 0) { dev_err(nor->dev, "error while writing configuration register\n"); @@ -1599,22 +1598,22 @@ static int spansion_read_cr_quad_enable(struct spi_nor *nor) */ static int sr2_bit7_quad_enable(struct spi_nor *nor) { - u8 sr2; + u8 *sr2 = nor->bouncebuf; int ret; /* Check current Quad Enable bit value. */ - ret = nor->read_reg(nor, SPINOR_OP_RDSR2, &sr2, 1); + ret = nor->read_reg(nor, SPINOR_OP_RDSR2, sr2, 1); if (ret) return ret; - if (sr2 & SR2_QUAD_EN_BIT7) + if (*sr2 & SR2_QUAD_EN_BIT7) return 0; /* Update the Quad Enable bit. */ - sr2 |= SR2_QUAD_EN_BIT7; + *sr2 |= SR2_QUAD_EN_BIT7; write_enable(nor); - ret = nor->write_reg(nor, SPINOR_OP_WRSR2, &sr2, 1); + ret = nor->write_reg(nor, SPINOR_OP_WRSR2, sr2, 1); if (ret < 0) { dev_err(nor->dev, "error while writing status register 2\n"); return -EINVAL; @@ -1627,8 +1626,8 @@ static int sr2_bit7_quad_enable(struct spi_nor *nor) } /* Read back and check it. */ - ret = nor->read_reg(nor, SPINOR_OP_RDSR2, &sr2, 1); - if (!(ret > 0 && (sr2 & SR2_QUAD_EN_BIT7))) { + ret = nor->read_reg(nor, SPINOR_OP_RDSR2, sr2, 1); + if (!(ret > 0 && (*sr2 & SR2_QUAD_EN_BIT7))) { dev_err(nor->dev, "SR2 Quad bit not set\n"); return -EINVAL; } @@ -2177,9 +2176,10 @@ static const struct flash_info spi_nor_ids[] = { static const struct flash_info *spi_nor_read_id(struct spi_nor *nor) { int tmp; - u8 id[SPI_NOR_MAX_ID_LEN]; + u8 *id; const struct flash_info *info; + id = nor->bouncebuf; tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, SPI_NOR_MAX_ID_LEN); if (tmp < 0) { dev_err(nor->dev, "error %d reading JEDEC ID\n", tmp); @@ -4121,6 +4121,16 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, nor->read_proto = SNOR_PROTO_1_1_1; nor->write_proto = SNOR_PROTO_1_1_1; + /* + * We need the bounce buffer early to read/write registers when going + * through the spi-mem layer (buffers have to be DMA-able). + */ + nor->bouncebuf_size = PAGE_SIZE; + nor->bouncebuf = devm_kmalloc(dev, nor->bouncebuf_size, + GFP_KERNEL); + if (!nor->bouncebuf) + return -ENOMEM; + if (name) info = spi_nor_match_id(name); /* Try to auto-detect if chip name wasn't specified or not found */ diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index 9f57cdfcc93d..6b5956a7a65a 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -344,6 +344,9 @@ struct flash_info; * @mtd: point to a mtd_info structure * @lock: the lock for the read/write/erase/lock/unlock operations * @dev: point to a spi device, or a spi nor controller device. + * @bouncebuf: bounce buffer used when the buffer passed by the MTD + * layer is not DMA-able + * @bouncebuf_size: size of the bounce buffer * @info: spi-nor part JDEC MFR id and other info * @page_size: the page size of the SPI NOR * @addr_width: number of address bytes @@ -356,7 +359,6 @@ struct flash_info; * @read_proto: the SPI protocol for read operations * @write_proto: the SPI protocol for write operations * @reg_proto the SPI protocol for read_reg/write_reg/erase operations - * @cmd_buf: used by the write_reg * @erase_map: the erase map of the SPI NOR * @prepare: [OPTIONAL] do some preparations for the * read/write/erase/lock/unlock operations @@ -382,6 +384,8 @@ struct spi_nor { struct mtd_info mtd; struct mutex lock; struct device *dev; + u8 *bouncebuf; + size_t bouncebuf_size; const struct flash_info *info; u32 page_size; u8 addr_width; @@ -394,7 +398,6 @@ struct spi_nor { enum spi_nor_protocol reg_proto; bool sst_write_second; u32 flags; - u8 cmd_buf[SPI_NOR_MAX_CMD_SIZE]; struct spi_nor_erase_map erase_map; int (*prepare)(struct spi_nor *nor, enum spi_nor_ops ops);