| Message ID | 6515c5ec-8432-0b20-426d-0428bbdf3712@cogentembedded.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] spi: add Renesas RPC-IF driver | expand |
On Wed, Jan 15, 2020, Sergei Shtylyov wrote: > Add the SPI driver for the Renesas RPC-IF. It's the "front end" driver > using the "back end" APIs in the main driver to talk to the real hardware. > We only implement the 'spi-mem' interface -- there's no need to implemebt > the usual SPI driver methods... I tried these patches on an RZ/A1H RSK board. At first, things were looking good. It would probe the SPI flash correctly and I could read out data. But, when I went to try and do an erase, it all went bad. Looking at the actual SPI lines, the commands coming out don't look like what I would expect from an MTD device. For example, I do a $ flash_eraseall /dev/mtd3 and all that comes out are Read Status commands (0x05). All the write enables and erase commands are missing. So, it looks like any command that is a write-only never actually sends anything. I did try and do a page program command: $ echo hello > /dev/mtd3 It sent the page program command (0x12), but in this case, it still didn't work because a write enable command was not sent first. I assume the MTD layer is requesting the correct sequence of commands, but Somehow this new driver is dropping of them. Chris
Hi Sergei,
As I mentioned, when testing on RZ/A1, some commands are not transmitted.
Basically and SPI command that did not have 'data' payload:
0x06: Write Enable
0xDC: Erase 256 kB (4-byte address)
0x04: Write Disable
The reason seems to be there is no case to set rpc->dir when there is no data.
So, it just gets left at whatever the last transfer was.
I added this fix and now it seems to work fine.
diff --git a/drivers/spi/spi-rpc-if.c b/drivers/spi/spi-rpc-if.c
index 0ff1a538bbd5..2165a0761844 100644
--- a/drivers/spi/spi-rpc-if.c
+++ b/drivers/spi/spi-rpc-if.c
@@ -53,6 +54,9 @@ static void rpcif_spi_mem_prepare(struct spi_device *spi_dev,
default:
break;
}
+ } else {
+ rpc_op.data.dir = RPCIF_NO_DATA;
+ rpc->dir = RPCIF_NO_DATA;
}
rpcif_prepare(rpc, &rpc_op, offs, len);
This seems like a bug that would effect add devices, not just the RZ/A1.
Side note, erase seems OK...but writing data seems to get messed up.
As you can see below, it's adding 2 bytes of 00 into the write stream.
$ flash_eraseall /dev/mtd3
Erasing 256 Kibyte @ 1000000 - 100% complete.
$ hexdump -C -n100 /dev/mtd3
00000000 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
*
00000060
$ hexdump -C -n100 /dev/mtd3
00000000 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
*
00000060
$ echo "hello" > /dev/mtd3
$ hexdump -C -n100 /dev/mtd3
00000000 68 65 6c 6c 00 00 6f 0a ff ff ff ff ff ff ff ff |hell..o.........|
00000010 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
*
00000060
Chris
Hello! On 02/08/2020 01:36 AM, Chris Brandt wrote: >> Add the SPI driver for the Renesas RPC-IF. It's the "front end" driver >> using the "back end" APIs in the main driver to talk to the real hardware. >> We only implement the 'spi-mem' interface -- there's no need to implemebt >> the usual SPI driver methods... > > I tried these patches on an RZ/A1H RSK board. > > At first, things were looking good. It would probe the SPI flash correctly > and I could read out data. > > But, when I went to try and do an erase, it all went bad. > Looking at the actual SPI lines, the commands coming out don't look like what > I would expect from an MTD device. > > For example, I do a > $ flash_eraseall /dev/mtd3 > and all that comes out are Read Status commands (0x05). > All the write enables and erase commands are missing. Well, I have warned that writes don't work. > So, it looks like any command that is a write-only never actually sends > anything. > > I did try and do a page program command: > $ echo hello > /dev/mtd3 > It sent the page program command (0x12), but in this case, it still didn't work > because a write enable command was not sent first. > I assume the MTD layer is requesting the correct sequence of commands, but > Somehow this new driver is dropping of them. Would explain what I saw (writes not surviving a JFFS2 remount)... > Chris MBR, Sergei
Hello Sergei, On Feb 10, 2020, Chris Brandt wrote: > Side note, erase seems OK...but writing data seems to get messed up. > As you can see below, it's adding 2 bytes of 00 into the write stream. > > $ flash_eraseall /dev/mtd3 > Erasing 256 Kibyte @ 1000000 - 100% complete. > $ hexdump -C -n100 /dev/mtd3 > 00000000 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff > |................| > * > 00000060 > $ hexdump -C -n100 /dev/mtd3 > 00000000 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff > |................| > * > 00000060 > $ echo "hello" > /dev/mtd3 > $ hexdump -C -n100 /dev/mtd3 > 00000000 68 65 6c 6c 00 00 6f 0a ff ff ff ff ff ff ff ff > |hell..o.........| > 00000010 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff > |................| > * > 00000060 I think here is the issue of the extra 00s in the write byte stream. For your code, you have this in function rpcif_io_xfer(): memcpy(data, rpc->buffer + pos, nbytes); if (nbytes > 4) { regmap_write(rpc->regmap, RPCIF_SMWDR1, data[0]); regmap_write(rpc->regmap, RPCIF_SMWDR0, data[1]); } else if (nbytes > 2) { regmap_write(rpc->regmap, RPCIF_SMWDR0, data[0]); } else { regmap_write(rpc->regmap, RPCIF_SMWDR0, data[0] << 16); } But, you cannot do a 32-bit register write when you have less than 32-bits of data. If you only have 2 bytes of data left to write, you have to do a 16-bit write to the SMWDR0 register. If you only have 1 byte of data left to write, you have to do a 8-bit write to the SMWDR0 register. If you only have 3 bytes of data left to write, you first send 2 bytes, then send the last byte. Your regmap is only set up to do 32-bit writes, so you'll have to use something like iowrite16 and iowrite8. This is why I did not use regmap in my SPI-BSC driver. For example, here is the code from my SPI-BSC driver: while (len > 0) { if (len >= 4) { unit = 4; smenr = SMENR_SPIDE(SPIDE_32BITS); } else { unit = len; if (unit == 3) unit = 2; if (unit >= 2) smenr = SMENR_SPIDE(SPIDE_16BITS); else smenr = SMENR_SPIDE(SPIDE_8BITS); } /* set 4bytes data, bit stream */ smwdr0 = *data++; if (unit >= 2) smwdr0 |= (u32)(*data++ << 8); if (unit >= 3) smwdr0 |= (u32)(*data++ << 16); if (unit >= 4) smwdr0 |= (u32)(*data++ << 24); /* mask unwrite area */ if (unit == 3) smwdr0 |= 0xFF000000; else if (unit == 2) smwdr0 |= 0xFFFF0000; else if (unit == 1) smwdr0 |= 0xFFFFFF00; /* write send data. */ if (unit == 2) spibsc_write16(sbsc, SMWDR0, (u16)smwdr0); else if (unit == 1) spibsc_write8(sbsc, SMWDR0, (u8)smwdr0); else spibsc_write(sbsc, SMWDR0, smwdr0); Chris
Index: spi/drivers/spi/Kconfig =================================================================== --- spi.orig/drivers/spi/Kconfig +++ spi/drivers/spi/Kconfig @@ -578,6 +578,12 @@ config SPI_RB4XX help SPI controller driver for the Mikrotik RB4xx series boards. +config SPI_RPCIF + tristate "Renesas RPC-IF SPI driver" + depends on RENESAS_RPCIF + help + SPI driver for Renesas R-Car Gen3 RPC-IF. + config SPI_RSPI tristate "Renesas RSPI/QSPI controller" depends on SUPERH || ARCH_RENESAS || COMPILE_TEST Index: spi/drivers/spi/Makefile =================================================================== --- spi.orig/drivers/spi/Makefile +++ spi/drivers/spi/Makefile @@ -87,6 +87,7 @@ obj-$(CONFIG_SPI_QCOM_QSPI) += spi-qcom obj-$(CONFIG_SPI_QUP) += spi-qup.o obj-$(CONFIG_SPI_ROCKCHIP) += spi-rockchip.o obj-$(CONFIG_SPI_RB4XX) += spi-rb4xx.o +obj-$(CONFIG_SPI_RPCIF) += spi-rpc-if.o obj-$(CONFIG_SPI_RSPI) += spi-rspi.o obj-$(CONFIG_SPI_S3C24XX) += spi-s3c24xx-hw.o spi-s3c24xx-hw-y := spi-s3c24xx.o Index: spi/drivers/spi/spi-rpc-if.c =================================================================== --- /dev/null +++ spi/drivers/spi/spi-rpc-if.c @@ -0,0 +1,213 @@ +// SPDX-License-Identifier: GPL-2.0 +// +// RPC-IF SPI/QSPI/Octa driver +// +// Copyright (C) 2018 ~ 2019 Renesas Solutions Corp. +// Copyright (C) 2019 Macronix International Co., Ltd. +// Copyright (C) 2019 Cogent Embedded, Inc. +// + +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/spi/spi.h> +#include <linux/spi/spi-mem.h> + +#include <memory/renesas-rpc-if.h> + +#include <asm/unaligned.h> + +static void rpcif_spi_mem_prepare(struct spi_device *spi_dev, + const struct spi_mem_op *spi_op, + u64 *offs, size_t *len) +{ + struct rpcif *rpc = spi_controller_get_devdata(spi_dev->controller); + struct rpcif_op rpc_op = { }; + + rpc_op.cmd.opcode = spi_op->cmd.opcode; + rpc_op.cmd.buswidth = spi_op->cmd.buswidth; + + if (spi_op->addr.nbytes) { + rpc_op.addr.buswidth = spi_op->addr.buswidth; + rpc_op.addr.nbytes = spi_op->addr.nbytes; + rpc_op.addr.val = spi_op->addr.val; + } + + if (spi_op->dummy.nbytes) { + rpc_op.dummy.buswidth = spi_op->dummy.buswidth; + rpc_op.dummy.ncycles = spi_op->dummy.nbytes * 8 / + spi_op->dummy.buswidth; + } + + if (spi_op->data.nbytes || (offs && len)) { + rpc_op.data.buswidth = spi_op->data.buswidth; + rpc_op.data.nbytes = spi_op->data.nbytes; + switch (spi_op->data.dir) { + case SPI_MEM_DATA_IN: + rpc_op.data.dir = RPCIF_DATA_IN; + rpc_op.data.buf.in = spi_op->data.buf.in; + break; + case SPI_MEM_DATA_OUT: + rpc_op.data.dir = RPCIF_DATA_OUT; + rpc_op.data.buf.out = spi_op->data.buf.out; + break; + default: + break; + } + } + + rpcif_prepare(rpc, &rpc_op, offs, len); +} + +static bool rpcif_spi_mem_supports_op(struct spi_mem *mem, + const struct spi_mem_op *op) +{ + if (!spi_mem_default_supports_op(mem, op)) + return false; + + if (op->data.buswidth > 4 || op->addr.buswidth > 4 || + op->dummy.buswidth > 4 || op->cmd.buswidth > 4 || + op->addr.nbytes > 4) + return false; + + return true; +} + +static ssize_t rpcif_spi_mem_dirmap_read(struct spi_mem_dirmap_desc *desc, + u64 offs, size_t len, void *buf) +{ + struct rpcif *rpc = + spi_controller_get_devdata(desc->mem->spi->controller); + + if (offs + desc->info.offset + len > U32_MAX) + return -EINVAL; + + rpcif_spi_mem_prepare(desc->mem->spi, &desc->info.op_tmpl, &offs, &len); + + return rpcif_dirmap_read(rpc, offs, len, buf); +} + +static int rpcif_spi_mem_dirmap_create(struct spi_mem_dirmap_desc *desc) +{ + struct rpcif *rpc = + spi_controller_get_devdata(desc->mem->spi->controller); + + if (desc->info.offset + desc->info.length > U32_MAX) + return -ENOTSUPP; + + if (!rpcif_spi_mem_supports_op(desc->mem, &desc->info.op_tmpl)) + return -ENOTSUPP; + + if (!rpc->dirmap && desc->info.op_tmpl.data.dir == SPI_MEM_DATA_IN) + return -ENOTSUPP; + + if (desc->info.op_tmpl.data.dir == SPI_MEM_DATA_OUT) + return -ENOTSUPP; + + return 0; +} + +static int rpcif_spi_mem_exec_op(struct spi_mem *mem, + const struct spi_mem_op *op) +{ + struct rpcif *rpc = + spi_controller_get_devdata(mem->spi->controller); + + rpcif_spi_mem_prepare(mem->spi, op, NULL, NULL); + + return rpcif_io_xfer(rpc); +} + +static const struct spi_controller_mem_ops rpcif_spi_mem_ops = { + .supports_op = rpcif_spi_mem_supports_op, + .exec_op = rpcif_spi_mem_exec_op, + .dirmap_create = rpcif_spi_mem_dirmap_create, + .dirmap_read = rpcif_spi_mem_dirmap_read, +}; + +static int rpcif_spi_probe(struct platform_device *pdev) +{ + struct device *parent = pdev->dev.parent; + struct spi_controller *ctlr; + struct rpcif *rpc; + int error; + + ctlr = spi_alloc_master(&pdev->dev, sizeof(*rpc)); + if (!ctlr) + return -ENOMEM; + + rpc = spi_controller_get_devdata(ctlr); + rpcif_sw_init(rpc, parent); + + platform_set_drvdata(pdev, ctlr); + + ctlr->dev.of_node = parent->of_node; + + rpcif_enable_rpm(rpc); + + ctlr->num_chipselect = 1; + ctlr->mem_ops = &rpcif_spi_mem_ops; + + ctlr->bits_per_word_mask = SPI_BPW_MASK(8); + ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_TX_QUAD | SPI_RX_QUAD; + ctlr->flags = SPI_CONTROLLER_HALF_DUPLEX; + + rpcif_hw_init(rpc, false); + + error = spi_register_controller(ctlr); + if (error) { + dev_err(&pdev->dev, "spi_register_controller failed\n"); + goto err_put_ctlr; + } + return 0; + +err_put_ctlr: + rpcif_disable_rpm(rpc); + spi_controller_put(ctlr); + + return error; +} + +static int rpcif_spi_remove(struct platform_device *pdev) +{ + struct spi_controller *ctlr = platform_get_drvdata(pdev); + struct rpcif *rpc = spi_controller_get_devdata(ctlr); + + spi_unregister_controller(ctlr); + rpcif_disable_rpm(rpc); + + return 0; +} + +#ifdef CONFIG_PM_SLEEP +static int rpcif_spi_suspend(struct device *dev) +{ + struct spi_controller *ctlr = dev_get_drvdata(dev); + + return spi_controller_suspend(ctlr); +} + +static int rpcif_spi_resume(struct device *dev) +{ + struct spi_controller *ctlr = dev_get_drvdata(dev); + + return spi_controller_resume(ctlr); +} + +static SIMPLE_DEV_PM_OPS(rpcif_spi_pm_ops, rpcif_spi_suspend, rpcif_spi_resume); +#define DEV_PM_OPS (&rpcif_spi_pm_ops) +#else +#define DEV_PM_OPS NULL +#endif + +static struct platform_driver rpcif_spi_driver = { + .probe = rpcif_spi_probe, + .remove = rpcif_spi_remove, + .driver = { + .name = "rpc-if-spi", + .pm = DEV_PM_OPS, + }, +}; +module_platform_driver(rpcif_spi_driver); + +MODULE_DESCRIPTION("Renesas RPC-IF SPI driver"); +MODULE_LICENSE("GPL v2");
Add the SPI driver for the Renesas RPC-IF. It's the "front end" driver using the "back end" APIs in the main driver to talk to the real hardware. We only implement the 'spi-mem' interface -- there's no need to implemebt the usual SPI driver methods... Based on the original patch by Mason Yang <masonccyang@mxic.com.tw>. Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- The patch is against the 'for-next' branch of Mark Brown's 'spi.git' repo. It depends on the memory RPC-IF driver (posted in December) in order to build/work: https://patchwork.kernel.org/patch/11283125/ https://patchwork.kernel.org/patch/11283127/ The known issue with writing to the JFFS2 file system (updates not surviving unmount/remount) were inherited from the original driver by Mason Yang... Changes in version 2: - removed unneeded transfer_one_message() method and the related code; - fixed up #include's as we switch from MFD to the memory core driver; - removed unneeded #include <linux/pm_runtime.h>; - removed 'struct rpcif_spi', replacing it with 'struct rpcif' everywhere; - added spi_mem_default_supports_op() call to rpcif_spi_mem_supports_op(); - added rpcif_sw_init() call in rpcif_spi_probe(); - set SPI_CONTROLLER_HALF_DUPLEX flag in rpcif_spi_probe(); - added a new variable 'struct device *parent' and renamed the 'ret' variable to 'error' in rpcif_spi_probe(); - changed the order of calls in the error path of rpcif_spi_probe() and in rpcif_spi_remove(); - changed from 'select' to 'depends on' the main driver Kconfig symbol, removed 'depends on ARCH_RENESAS || COMPILE TEST'; - renamed rpcif_spi_mem_set_prep_op_cfg() to rpcif_spi_mem_prepare(), updated the rpcif_io_xfer() call there to match the RPC-IF core driver, changed 'rpc_op' there from parameter into the local variable; - changed the platform driver's name to "rpc-if-spi"; - fixed whitespace in rpcif_spi_mem_exec_op()'s prototype; - beautified the whitespace in the initializers of 'rpcif_spi_mem_ops' and 'rpcif_spi_driver'; - changed the heading comment from /* */ to //; - updated the patch description with more details. drivers/spi/Kconfig | 6 + drivers/spi/Makefile | 1 drivers/spi/spi-rpc-if.c | 213 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 220 insertions(+)