From: Boris Brezillon <boris.brezillon@collabora.com>
To: Vignesh Raghavendra <vigneshr@ti.com>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>,
Richard Weinberger <richard@nod.at>,
Tudor Ambarus <tudor.ambarus@microchip.com>,
Marek Vasut <marek.vasut@gmail.com>,
Boris Brezillon <bbrezillon@kernel.org>,
<linux-kernel@vger.kernel.org>, <linux-mtd@lists.infradead.org>
Subject: Re: [PATCH v3 1/3] mtd: spi-nor: always use bounce buffer for register read/writes
Date: Thu, 1 Aug 2019 07:46:47 +0200 [thread overview]
Message-ID: <20190801074647.792479c1@collabora.com> (raw)
In-Reply-To: <20190801043052.30192-2-vigneshr@ti.com>
On Thu, 1 Aug 2019 10:00:50 +0530
Vignesh Raghavendra <vigneshr@ti.com> 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@ti.com>
> ---
> 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@collabora.com>
> + */
> + 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 prev parent reply other threads:[~2019-08-01 5:46 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-01 4:30 [PATCH v3 0/3] Merge m25p80 into spi-nor Vignesh Raghavendra
2019-08-01 4:30 ` [PATCH v3 1/3] mtd: spi-nor: always use bounce buffer for register read/writes Vignesh Raghavendra
2019-08-01 5:46 ` Boris Brezillon [this message]
2019-08-01 6:45 ` Vignesh Raghavendra
2019-08-01 4:30 ` [PATCH v3 2/3] mtd: spi-nor: Move m25p80 code in spi-nor.c Vignesh Raghavendra
2019-08-01 5:52 ` Boris Brezillon
2019-08-01 6:46 ` Vignesh Raghavendra
2019-08-01 4:30 ` [PATCH v3 3/3] mtd: spi-nor: Rework hwcaps selection for the spi-mem case Vignesh Raghavendra
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190801074647.792479c1@collabora.com \
--to=boris.brezillon@collabora.com \
--cc=bbrezillon@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=marek.vasut@gmail.com \
--cc=miquel.raynal@bootlin.com \
--cc=richard@nod.at \
--cc=tudor.ambarus@microchip.com \
--cc=vigneshr@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).