Re: [PATCH v3 1/3] mtd: spi-nor: always use bounce buffer for register read/writes

From: Boris Brezillon
Date: Thu Aug 01 2019 - 01:46:59 EST

Next message: Valdis KlÄtnieks: "Re: [PATCH] linux-next 20190731 - aegis128-core.c fails to build"
Previous message: Andrew Jeffery: "Re: [PATCH 0/3] ARM: dts: aspeed: Deprecate g[45]-style compatibles"
In reply to: Vignesh Raghavendra: "[PATCH v3 1/3] mtd: spi-nor: always use bounce buffer for register read/writes"
Next in thread: Vignesh Raghavendra: "Re: [PATCH v3 1/3] mtd: spi-nor: always use bounce buffer for register read/writes"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, 1 Aug 2019 10:00:50 +0530
Vignesh Raghavendra <vigneshr@xxxxxx> wrote:

> 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.
>
> Signed-off-by: Vignesh Raghavendra <vigneshr@xxxxxx>
> ---
> v3: new patch
>
> drivers/mtd/spi-nor/spi-nor.c | 71 ++++++++++++++++++++---------------
> include/linux/mtd/spi-nor.h | 7 +++-
> 2 files changed, 46 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 03cc788511d5..8685e4ab6a25 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;
> }
> @@ -2180,11 +2179,13 @@ static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
> u8 id[SPI_NOR_MAX_ID_LEN];
> const struct flash_info *info;
>
> - tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, SPI_NOR_MAX_ID_LEN);
> + tmp = nor->read_reg(nor, SPINOR_OP_RDID, nor->bouncebuf,
> + SPI_NOR_MAX_ID_LEN);
> if (tmp < 0) {
> dev_err(nor->dev, "error %d reading JEDEC ID\n", tmp);
> return ERR_PTR(tmp);
> }
> + memcpy(id, nor->bouncebuf, SPI_NOR_MAX_ID_LEN);

Why not directly including the change you have in patch 2 (id is a
pointer that points directly to ->bouncebuf) so you can get rid of this
memcpy() here?

>
> for (tmp = 0; tmp < ARRAY_SIZE(spi_nor_ids) - 1; tmp++) {
> info = &spi_nor_ids[tmp];
> @@ -4121,6 +4122,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).

You should probably extend this comment in patch 2 to explain why 4k
should be enough for regular read/write operations.

The patch looks good otherwise.

Reviewed-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>

> + */
> + 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);

Next message: Valdis KlÄtnieks: "Re: [PATCH] linux-next 20190731 - aegis128-core.c fails to build"
Previous message: Andrew Jeffery: "Re: [PATCH 0/3] ARM: dts: aspeed: Deprecate g[45]-style compatibles"
In reply to: Vignesh Raghavendra: "[PATCH v3 1/3] mtd: spi-nor: always use bounce buffer for register read/writes"
Next in thread: Vignesh Raghavendra: "Re: [PATCH v3 1/3] mtd: spi-nor: always use bounce buffer for register read/writes"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]