2019-03-29 08:14:26

by Mason Yang

[permalink] [raw]
Subject: [PATCH v9 0/3] mfd: Add Renesas R-Car Gen3 RPC-IF MFD driver

Hi,

v9 patch is for RPC MFD driver and RPC SPI driver.

v8 patch including:
1) Supported SoC-specific values in DTS.
2) Rename device node name as flash.

v7 patch is according to Geert and Sergei's comments:
1) Add all R-Car Gen3 model in dts.
2) patch rpc-if child node search.
3) minror coding style.

v6 patch is accroding to Geert, Marek and Sergei's comments:
1) spi_controller for new code.
2) "renesas,rcar-gen3-rpc" instead of "renesas,r8a77995-rpc."
3) patch external address read mode w/o u64 readq().
4) patch dts for write buffer & drop "renesas,rpc-mode".
5) coding style and so on.

v5 patch is accroding to Sergei's comments:
1) Read 6 bytes ID from Sergei's patch.
2) regmap_update_bits().
3) C++ style comment.

v4 patch is according to Sergei's comments including:
1) Drop soc_device_match().
2) Drop unused RPC registers.
3) Use ilog2() instead of fls().
4) Patch read 6 bytes ID w/ one command.
5) Coding style and so on.

v3 patch is according to Marek and Geert's comments including:
1) soc_device_mach() to set up RPC_PHYCNT_STRTIM.
2) get_unaligned().
3) rpc-mode for rpi-spi-flash or rpc-hyperflash.
4) coding style and so on.

v2 patch including:
1) remove RPC clock enable/dis-able control,
2) patch run time PM.
3) add RPC module software reset,
4) add regmap.
5) other coding style and so on.

thanks for your review.

best regards,
Mason

Mason Yang (3):
mfd: Add Renesas R-Car Gen3 RPC-IF MFD controller driver
spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver
dt-bindings: mfd: Document Renesas R-Car Gen3 RPC-IF controller
bindings

.../devicetree/bindings/mfd/mfd-renesas-rpc.txt | 57 ++
drivers/mfd/Kconfig | 9 +
drivers/mfd/Makefile | 1 +
drivers/mfd/renesas-rpc.c | 140 +++++
drivers/spi/Kconfig | 6 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-renesas-rpc.c | 640 +++++++++++++++++++++
include/linux/mfd/renesas-rpc.h | 154 +++++
8 files changed, 1008 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
create mode 100644 drivers/mfd/renesas-rpc.c
create mode 100644 drivers/spi/spi-renesas-rpc.c
create mode 100644 include/linux/mfd/renesas-rpc.h

--
1.9.1



2019-03-29 08:12:51

by Mason Yang

[permalink] [raw]
Subject: [PATCH v9 3/3] dt-bindings: mfd: Document Renesas R-Car Gen3 RPC-IF controller bindings

Document the bindings used by the Renesas R-Car Gen3 RPC-IF MFD controller.

Signed-off-by: Mason Yang <[email protected]>
---
.../devicetree/bindings/mfd/mfd-renesas-rpc.txt | 57 ++++++++++++++++++++++
1 file changed, 57 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt

diff --git a/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
new file mode 100644
index 0000000..577986b
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
@@ -0,0 +1,57 @@
+Renesas R-Car Gen3 RPC-IF MFD controller Device Tree Bindings
+---------------------------------------------------------------------
+
+Required properties:
+- compatible: should be an SoC-specific compatible value, followed by
+ "renesas,rcar-gen3-rpc" as a fallback.
+ supported SoC-specific values are:
+ "renesas,r8a77995-rpc" (R-Car D3)
+- reg: should contain three register areas:
+ first for the base address of rpc-if registers,
+ second for the direct mapping read mode and
+ third for the write buffer area.
+- reg-names: should contain "regs", "dirmap" and "wbuf"
+- clocks: should contain 1 entries for the module's clock
+- clock-names: should contain "rpc"
+- #address-cells: should be 1
+- #size-cells: should be 0
+
+Required nodes:
+ spi:
+ Node for configuring the SPI controller driver.
+ Required properties:
+ compatible = "renesas,rcar-rpc-spi";
+
+ hf:
+ Node for configuring the hyperflash controller driver.
+ Required properties:
+ compatible = "renesas,rcar-rpc-hf";
+
+Example:
+
+ rpc_mfd: rpc-mfd@ee200000 {
+ compatible = "renesas,r8a77995-rpc", "renesas,rcar-gen3-rpc";
+ reg = <0 0xee200000 0 0x200>, <0 0x08000000 0 0x4000000>,
+ <0 0xee208000 0 0x100>;
+ reg-names = "regs", "dirmap", "wbuf";
+ clocks = <&cpg CPG_MOD 917>;
+ clock-names = "rpc";
+ power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;
+ resets = <&cpg 917>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ spi {
+ compatible = "renesas,rcar-rpc-spi";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ flash@0 {
+ compatible = "jedec,spi-nor";
+ reg = <0>;
+ spi-max-frequency = <40000000>;
+ spi-tx-bus-width = <1>;
+ spi-rx-bus-width = <1>;
+ };
+ };
+ };
--
1.9.1


2019-03-29 08:13:02

by Mason Yang

[permalink] [raw]
Subject: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver

Add a driver for Renesas R-Car Gen3 RPC-IF SPI controller.

Signed-off-by: Mason Yang <[email protected]>
Signed-off-by: Sergei Shtylyov <[email protected]>
---
drivers/spi/Kconfig | 6 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-renesas-rpc.c | 640 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 647 insertions(+)
create mode 100644 drivers/spi/spi-renesas-rpc.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index f761655..1f52bcf 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -564,6 +564,12 @@ config SPI_RSPI
help
SPI driver for Renesas RSPI and QSPI blocks.

+config SPI_RENESAS_RPC
+ tristate "Renesas R-Car Gen3 RPC-IF controller"
+ depends on ARCH_RENESAS || COMPILE_TEST
+ help
+ SPI driver for Renesas R-Car Gen3 RPC-IF.
+
config SPI_QCOM_QSPI
tristate "QTI QSPI controller"
depends on ARCH_QCOM
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index d8fc03c..b3a3deb 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -86,6 +86,7 @@ obj-$(CONFIG_SPI_QUP) += spi-qup.o
obj-$(CONFIG_SPI_ROCKCHIP) += spi-rockchip.o
obj-$(CONFIG_SPI_RB4XX) += spi-rb4xx.o
obj-$(CONFIG_SPI_RSPI) += spi-rspi.o
+obj-$(CONFIG_SPI_RENESAS_RPC) += spi-renesas-rpc.o
obj-$(CONFIG_SPI_S3C24XX) += spi-s3c24xx-hw.o
spi-s3c24xx-hw-y := spi-s3c24xx.o
spi-s3c24xx-hw-$(CONFIG_SPI_S3C24XX_FIQ) += spi-s3c24xx-fiq.o
diff --git a/drivers/spi/spi-renesas-rpc.c b/drivers/spi/spi-renesas-rpc.c
new file mode 100644
index 0000000..037f273
--- /dev/null
+++ b/drivers/spi/spi-renesas-rpc.c
@@ -0,0 +1,640 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (C) 2018 ~ 2019 Renesas Solutions Corp.
+// Copyright (C) 2019 Macronix International Co., Ltd.
+//
+// R-Car Gen3 RPC-IF SPI/QSPI/Octa driver
+//
+// Author:
+// Mason Yang <[email protected]>
+//
+
+#include <linux/mfd/renesas-rpc.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/spi-mem.h>
+
+#include <asm/unaligned.h>
+
+struct rpc_spi {
+ struct clk *clk_rpc;
+ void __iomem *base;
+ void __iomem *dirmap;
+ void __iomem *wbuf;
+ struct regmap *regmap;
+ u32 cur_speed_hz;
+ u32 cmd;
+ u32 addr;
+ u32 dummy;
+ u32 smcr;
+ u32 smenr;
+ u32 xferlen;
+ u32 totalxferlen;
+ enum spi_mem_data_dir xfer_dir;
+ struct reset_control *rstc;
+};
+
+static int rpc_spi_set_freq(struct rpc_spi *rpc, unsigned long freq)
+{
+ int ret;
+
+ if (rpc->cur_speed_hz == freq)
+ return 0;
+
+ ret = clk_set_rate(rpc->clk_rpc, freq);
+ if (ret)
+ return ret;
+
+ rpc->cur_speed_hz = freq;
+ return ret;
+}
+
+static void rpc_spi_hw_init(struct rpc_spi *rpc)
+{
+ //
+ // NOTE: The 0x260 are undocumented bits, but they must be set.
+ // RPC_PHYCNT_STRTIM is strobe timing adjustment bit,
+ // 0x0 : the delay is biggest,
+ // 0x1 : the delay is 2nd biggest,
+ // On H3 ES1.x, the value should be 0, while on others,
+ // the value should be 6.
+ //
+ regmap_write(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_CAL |
+ RPC_PHYCNT_STRTIM(6) | 0x260);
+
+ //
+ // NOTE: The 0x1511144 are undocumented bits, but they must be set
+ // for RPC_PHYOFFSET1.
+ // The 0x31 are undocumented bits, but they must be set
+ // for RPC_PHYOFFSET2.
+ //
+ regmap_write(rpc->regmap, RPC_PHYOFFSET1, RPC_PHYOFFSET1_DDRTMG(3) |
+ 0x1511144);
+ regmap_write(rpc->regmap, RPC_PHYOFFSET2, 0x31 |
+ RPC_PHYOFFSET2_OCTTMG(4));
+ regmap_write(rpc->regmap, RPC_SSLDR, RPC_SSLDR_SPNDL(7) |
+ RPC_SSLDR_SLNDL(7) | RPC_SSLDR_SCKDL(7));
+ regmap_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD | RPC_CMNCR_SFDE |
+ RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
+ RPC_CMNCR_BSZ(0));
+}
+
+static int wait_msg_xfer_end(struct rpc_spi *rpc)
+{
+ u32 sts;
+
+ return regmap_read_poll_timeout(rpc->regmap, RPC_CMNSR, sts,
+ sts & RPC_CMNSR_TEND, 0, USEC_PER_SEC);
+}
+
+static u8 rpc_bits_set(u32 nbytes)
+{
+ nbytes = clamp(nbytes, 1U, 4U);
+
+ return GENMASK(3, 4 - nbytes);
+}
+
+static int rpc_spi_io_xfer(struct rpc_spi *rpc,
+ const void *tx_buf, void *rx_buf)
+{
+ u32 smenr, smcr, data, pos = 0;
+ int ret;
+
+ regmap_update_bits(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD, RPC_CMNCR_MD);
+ regmap_write(rpc->regmap, RPC_SMDRENR, 0);
+ regmap_write(rpc->regmap, RPC_SMCMR, rpc->cmd);
+ regmap_write(rpc->regmap, RPC_SMDMCR, rpc->dummy);
+ regmap_write(rpc->regmap, RPC_SMADR, rpc->addr);
+ smenr = rpc->smenr;
+
+ if (tx_buf) {
+ while (pos < rpc->xferlen) {
+ u32 nbytes = rpc->xferlen - pos;
+
+ regmap_write(rpc->regmap, RPC_SMWDR0,
+ get_unaligned((u32 *)(tx_buf + pos)));
+
+ smcr = rpc->smcr | RPC_SMCR_SPIE;
+
+ if (nbytes > 4) {
+ nbytes = 4;
+ smcr |= RPC_SMCR_SSLKP;
+ }
+
+ regmap_write(rpc->regmap, RPC_SMENR, smenr);
+ regmap_write(rpc->regmap, RPC_SMCR, smcr);
+ ret = wait_msg_xfer_end(rpc);
+ if (ret)
+ goto err_out;
+
+ pos += nbytes;
+ smenr = rpc->smenr & ~RPC_SMENR_CDE &
+ ~RPC_SMENR_ADE(0xf);
+ }
+ } else if (rx_buf) {
+ //
+ // RPC-IF spoils the data for the commands without an address
+ // phase (like RDID) in the manual mode, so we'll have to work
+ // around this issue by using the external address space read
+ // mode instead.
+ //
+ if (!(smenr & RPC_SMENR_ADE(0xf)) && rpc->dirmap) {
+ regmap_update_bits(rpc->regmap, RPC_CMNCR,
+ RPC_CMNCR_MD, 0);
+ regmap_write(rpc->regmap, RPC_DRCR,
+ RPC_DRCR_RBURST(32) | RPC_DRCR_RBE);
+ regmap_write(rpc->regmap, RPC_DREAR, RPC_DREAR_EAC(1));
+ regmap_write(rpc->regmap, RPC_DRCMR, rpc->cmd);
+ regmap_write(rpc->regmap, RPC_DRDMCR, rpc->dummy);
+ regmap_write(rpc->regmap, RPC_DROPR, 0);
+ regmap_write(rpc->regmap, RPC_DRENR, smenr);
+ memcpy_fromio(rx_buf, rpc->dirmap, rpc->xferlen);
+ regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RCF);
+ } else {
+ while (pos < rpc->xferlen) {
+ u32 nbytes = rpc->xferlen - pos;
+
+ if (nbytes > 4)
+ nbytes = 4;
+
+ regmap_write(rpc->regmap, RPC_SMENR, smenr);
+ regmap_write(rpc->regmap, RPC_SMCR, rpc->smcr |
+ RPC_SMCR_SPIE);
+ ret = wait_msg_xfer_end(rpc);
+ if (ret)
+ goto err_out;
+
+ regmap_read(rpc->regmap, RPC_SMRDR0, &data);
+ memcpy(rx_buf + pos, &data, nbytes);
+ pos += nbytes;
+
+ regmap_write(rpc->regmap, RPC_SMADR,
+ rpc->addr + pos);
+ }
+ }
+ } else {
+ regmap_write(rpc->regmap, RPC_SMENR, rpc->smenr);
+ regmap_write(rpc->regmap, RPC_SMCR, rpc->smcr | RPC_SMCR_SPIE);
+ ret = wait_msg_xfer_end(rpc);
+ if (ret)
+ goto err_out;
+ }
+
+ return 0;
+
+err_out:
+ return reset_control_reset(rpc->rstc);
+}
+
+static void rpc_spi_mem_set_prep_op_cfg(struct spi_device *spi,
+ const struct spi_mem_op *op,
+ u64 *offs, size_t *len)
+{
+ struct rpc_spi *rpc = spi_controller_get_devdata(spi->controller);
+
+ rpc->cmd = RPC_SMCMR_CMD(op->cmd.opcode);
+ rpc->smenr = RPC_SMENR_CDE |
+ RPC_SMENR_CDB(ilog2(op->cmd.buswidth));
+ rpc->totalxferlen = 1;
+ rpc->xfer_dir = SPI_MEM_NO_DATA;
+ rpc->xferlen = 0;
+ rpc->addr = 0;
+
+ if (op->addr.nbytes) {
+ rpc->smenr |= RPC_SMENR_ADB(ilog2(op->addr.buswidth));
+ if (op->addr.nbytes == 4)
+ rpc->smenr |= RPC_SMENR_ADE(0xf);
+ else
+ rpc->smenr |= RPC_SMENR_ADE(0x7);
+
+ if (offs && len)
+ rpc->addr = *offs;
+ else
+ rpc->addr = op->addr.val;
+ rpc->totalxferlen += op->addr.nbytes;
+ }
+
+ if (op->dummy.nbytes) {
+ rpc->smenr |= RPC_SMENR_DME;
+ rpc->dummy = RPC_SMDMCR_DMCYC(op->dummy.nbytes);
+ rpc->totalxferlen += op->dummy.nbytes;
+ }
+
+ if (op->data.nbytes || (offs && len)) {
+ switch (op->data.dir) {
+ case SPI_MEM_DATA_IN:
+ rpc->smcr = RPC_SMCR_SPIRE;
+ rpc->xfer_dir = SPI_MEM_DATA_IN;
+ break;
+ case SPI_MEM_DATA_OUT:
+ rpc->smcr = RPC_SMCR_SPIWE;
+ rpc->xfer_dir = SPI_MEM_DATA_OUT;
+ break;
+ default:
+ break;
+ }
+
+ if (offs && len) {
+ rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_set(*len)) |
+ RPC_SMENR_SPIDB(ilog2(op->data.buswidth));
+ rpc->xferlen = *len;
+ rpc->totalxferlen += *len;
+ } else {
+ rpc->smenr |=
+ RPC_SMENR_SPIDE(rpc_bits_set(op->data.nbytes)) |
+ RPC_SMENR_SPIDB(ilog2(op->data.buswidth));
+ rpc->xferlen = op->data.nbytes;
+ rpc->totalxferlen += op->data.nbytes;
+ }
+ }
+}
+
+static bool rpc_spi_mem_supports_op(struct spi_mem *mem,
+ const struct spi_mem_op *op)
+{
+ 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 rpc_spi_mem_dirmap_read(struct spi_mem_dirmap_desc *desc,
+ u64 offs, size_t len, void *buf)
+{
+ struct rpc_spi *rpc =
+ spi_controller_get_devdata(desc->mem->spi->controller);
+ int ret;
+
+ if (offs + desc->info.offset + len > U32_MAX)
+ return -EINVAL;
+
+ if (len > 0x4000000)
+ len = 0x4000000;
+
+ ret = rpc_spi_set_freq(rpc, desc->mem->spi->max_speed_hz);
+ if (ret)
+ return ret;
+
+ rpc_spi_mem_set_prep_op_cfg(desc->mem->spi,
+ &desc->info.op_tmpl, &offs, &len);
+
+ regmap_update_bits(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD, 0);
+ regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RBURST(32) |
+ RPC_DRCR_RBE);
+
+ regmap_write(rpc->regmap, RPC_DRCMR, rpc->cmd);
+ regmap_write(rpc->regmap, RPC_DREAR, RPC_DREAR_EAC(1));
+ regmap_write(rpc->regmap, RPC_DROPR, 0);
+ regmap_write(rpc->regmap, RPC_DRENR, rpc->smenr);
+ regmap_write(rpc->regmap, RPC_DRDMCR, rpc->dummy);
+ regmap_write(rpc->regmap, RPC_DRDRENR, 0);
+
+ memcpy_fromio(buf, rpc->dirmap + desc->info.offset + offs, len);
+
+ return len;
+}
+
+static ssize_t rpc_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
+ u64 offs, size_t len, const void *buf)
+{
+ struct rpc_spi *rpc =
+ spi_controller_get_devdata(desc->mem->spi->controller);
+ int ret;
+
+ if (offs + desc->info.offset + len > U32_MAX)
+ return -EINVAL;
+
+ if (len > RPC_WBUF_SIZE)
+ len = RPC_WBUF_SIZE;
+
+ ret = rpc_spi_set_freq(rpc, desc->mem->spi->max_speed_hz);
+ if (ret)
+ return ret;
+
+ rpc_spi_mem_set_prep_op_cfg(desc->mem->spi,
+ &desc->info.op_tmpl, &offs, &len);
+
+ regmap_update_bits(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD, RPC_CMNCR_MD);
+
+ regmap_write(rpc->regmap, RPC_SMDRENR, 0);
+
+ regmap_update_bits(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_STRTIM(7) |
+ RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF,
+ RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF);
+
+ memcpy_toio(rpc->wbuf, buf, len);
+
+ regmap_write(rpc->regmap, RPC_SMCMR, rpc->cmd);
+ regmap_write(rpc->regmap, RPC_SMADR, offs);
+ regmap_write(rpc->regmap, RPC_SMENR, rpc->smenr);
+ regmap_write(rpc->regmap, RPC_SMCR, rpc->smcr | RPC_SMCR_SPIE);
+ ret = wait_msg_xfer_end(rpc);
+ if (ret)
+ goto err_out;
+
+ regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RCF);
+
+ regmap_update_bits(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_STRTIM(7) |
+ RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF,
+ RPC_PHYCNT_STRTIM(6));
+
+ return len;
+
+err_out:
+ return reset_control_reset(rpc->rstc);
+}
+
+static int rpc_spi_mem_dirmap_create(struct spi_mem_dirmap_desc *desc)
+{
+ struct rpc_spi *rpc =
+ spi_controller_get_devdata(desc->mem->spi->controller);
+
+ if (desc->info.offset + desc->info.length > U32_MAX)
+ return -ENOTSUPP;
+
+ if (!rpc_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 (!rpc->wbuf &&
+ desc->info.op_tmpl.data.dir == SPI_MEM_DATA_OUT)
+ return -ENOTSUPP;
+
+ return 0;
+}
+
+static int rpc_spi_mem_exec_op(struct spi_mem *mem,
+ const struct spi_mem_op *op)
+{
+ struct rpc_spi *rpc = spi_controller_get_devdata(mem->spi->controller);
+ int ret;
+
+ ret = rpc_spi_set_freq(rpc, mem->spi->max_speed_hz);
+ if (ret)
+ return ret;
+
+ rpc_spi_mem_set_prep_op_cfg(mem->spi, op, NULL, NULL);
+
+ ret = rpc_spi_io_xfer(rpc,
+ op->data.dir == SPI_MEM_DATA_OUT ?
+ op->data.buf.out : NULL,
+ op->data.dir == SPI_MEM_DATA_IN ?
+ op->data.buf.in : NULL);
+
+ return ret;
+}
+
+static const struct spi_controller_mem_ops rpc_spi_mem_ops = {
+ .supports_op = rpc_spi_mem_supports_op,
+ .exec_op = rpc_spi_mem_exec_op,
+ .dirmap_create = rpc_spi_mem_dirmap_create,
+ .dirmap_read = rpc_spi_mem_dirmap_read,
+ .dirmap_write = rpc_spi_mem_dirmap_write,
+};
+
+static void rpc_spi_transfer_setup(struct rpc_spi *rpc,
+ struct spi_message *msg)
+{
+ struct spi_transfer *t, xfer[4] = { };
+ u32 i, xfercnt, xferpos = 0;
+
+ rpc->totalxferlen = 0;
+ rpc->xfer_dir = SPI_MEM_NO_DATA;
+
+ list_for_each_entry(t, &msg->transfers, transfer_list) {
+ if (t->tx_buf) {
+ xfer[xferpos].tx_buf = t->tx_buf;
+ xfer[xferpos].tx_nbits = t->tx_nbits;
+ }
+
+ if (t->rx_buf) {
+ xfer[xferpos].rx_buf = t->rx_buf;
+ xfer[xferpos].rx_nbits = t->rx_nbits;
+ }
+
+ if (t->len) {
+ xfer[xferpos++].len = t->len;
+ rpc->totalxferlen += t->len;
+ }
+
+ if (list_is_last(&t->transfer_list, &msg->transfers)) {
+ if (xferpos > 1) {
+ if (t->rx_buf) {
+ rpc->xfer_dir = SPI_MEM_DATA_IN;
+ rpc->smcr = RPC_SMCR_SPIRE;
+ } else if (t->tx_buf) {
+ rpc->xfer_dir = SPI_MEM_DATA_OUT;
+ rpc->smcr = RPC_SMCR_SPIWE;
+ }
+ }
+ }
+ }
+
+ xfercnt = xferpos;
+ rpc->xferlen = xfer[--xferpos].len;
+ rpc->cmd = RPC_SMCMR_CMD(((u8 *)xfer[0].tx_buf)[0]);
+ rpc->smenr = RPC_SMENR_CDE |
+ RPC_SMENR_CDB(ilog2((unsigned int)xfer[0].tx_nbits));
+ rpc->addr = 0;
+
+ if (xfercnt > 2 && xfer[1].len && xfer[1].tx_buf) {
+ rpc->smenr |=
+ RPC_SMENR_ADB(ilog2((unsigned int)xfer[1].tx_nbits));
+
+ for (i = 0; i < xfer[1].len; i++)
+ rpc->addr |= ((u8 *)xfer[1].tx_buf)[i] <<
+ (8 * (xfer[1].len - i - 1));
+
+ if (xfer[1].len == 4)
+ rpc->smenr |= RPC_SMENR_ADE(0xf);
+ else
+ rpc->smenr |= RPC_SMENR_ADE(0x7);
+ }
+
+ if (xfercnt > 3 && xfer[2].len && xfer[2].tx_buf) {
+ rpc->smenr |= RPC_SMENR_DME;
+ rpc->dummy = RPC_SMDMCR_DMCYC(xfer[2].len);
+ }
+
+ for (i = xfercnt - 1; i < xfercnt && xfercnt > 1; i++) {
+ if (xfer[i].rx_buf) {
+ rpc->smenr |=
+ RPC_SMENR_SPIDE(rpc_bits_set(xfer[i].len)) |
+ RPC_SMENR_SPIDB(ilog2
+ ((unsigned int)xfer[i].rx_nbits));
+ } else if (xfer[i].tx_buf) {
+ rpc->smenr |=
+ RPC_SMENR_SPIDE(rpc_bits_set(xfer[i].len)) |
+ RPC_SMENR_SPIDB(ilog2
+ ((unsigned int)xfer[i].tx_nbits));
+ }
+ }
+}
+
+static inline int rpc_spi_xfer_message(struct rpc_spi *rpc,
+ struct spi_transfer *data_xfer)
+{
+ int ret;
+
+ ret = rpc_spi_set_freq(rpc, data_xfer->speed_hz);
+ if (ret)
+ return ret;
+
+ ret = rpc_spi_io_xfer(rpc,
+ rpc->xfer_dir == SPI_MEM_DATA_OUT ?
+ data_xfer->tx_buf : NULL,
+ rpc->xfer_dir == SPI_MEM_DATA_IN ?
+ data_xfer->rx_buf : NULL);
+
+ return ret;
+}
+
+static int rpc_spi_transfer_one_message(struct spi_controller *ctlr,
+ struct spi_message *msg)
+{
+ struct rpc_spi *rpc = spi_controller_get_devdata(ctlr);
+ struct spi_transfer *data_xfer;
+ int ret;
+
+ rpc_spi_transfer_setup(rpc, msg);
+
+ data_xfer = list_last_entry(&msg->transfers, struct spi_transfer,
+ transfer_list);
+
+ ret = rpc_spi_xfer_message(rpc, data_xfer);
+ if (ret)
+ goto out;
+
+ msg->status = 0;
+ msg->actual_length = rpc->totalxferlen;
+out:
+ spi_finalize_current_message(ctlr);
+ return 0;
+}
+
+static int rpc_spi_probe(struct platform_device *pdev)
+{
+ struct spi_controller *ctlr;
+ struct rpc_mfd *rpc_mfd = dev_get_drvdata(pdev->dev.parent);
+ struct rpc_spi *rpc;
+ int ret;
+
+ ctlr = spi_alloc_master(&pdev->dev, sizeof(*rpc));
+ if (!ctlr)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, ctlr);
+
+ rpc = spi_controller_get_devdata(ctlr);
+
+ ctlr->dev.of_node = pdev->dev.of_node;
+
+ rpc->clk_rpc = rpc_mfd->clk_rpc;
+ if (IS_ERR(rpc->clk_rpc))
+ return PTR_ERR(rpc->clk_rpc);
+
+ rpc->base = rpc_mfd->base;
+ if (IS_ERR(rpc->base))
+ return PTR_ERR(rpc->base);
+
+ rpc->regmap = rpc_mfd->regmap;
+ if (IS_ERR(rpc->regmap)) {
+ dev_err(&pdev->dev,
+ "failed to regmap for rpc-spi, error %ld\n",
+ PTR_ERR(rpc->regmap));
+ return PTR_ERR(rpc->regmap);
+ }
+
+ rpc->dirmap = rpc_mfd->dirmap;
+ if (IS_ERR(rpc->dirmap))
+ rpc->dirmap = NULL;
+
+ rpc->wbuf = rpc_mfd->wbuf;
+ if (IS_ERR(rpc->wbuf))
+ rpc->wbuf = NULL;
+
+ rpc->rstc = rpc_mfd->rstc;
+ if (IS_ERR(rpc->rstc))
+ return PTR_ERR(rpc->rstc);
+
+ pm_runtime_enable(&pdev->dev);
+ ctlr->auto_runtime_pm = true;
+
+ ctlr->num_chipselect = 1;
+ ctlr->mem_ops = &rpc_spi_mem_ops;
+ ctlr->transfer_one_message = rpc_spi_transfer_one_message;
+
+ ctlr->bits_per_word_mask = SPI_BPW_MASK(8);
+ ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_TX_QUAD | SPI_RX_QUAD;
+
+ rpc_spi_hw_init(rpc);
+
+ ret = spi_register_controller(ctlr);
+ if (ret) {
+ dev_err(&pdev->dev, "spi_register_controller failed\n");
+ goto err_put_ctlr;
+ }
+ return 0;
+
+err_put_ctlr:
+ spi_controller_put(ctlr);
+ pm_runtime_disable(&pdev->dev);
+
+ return ret;
+}
+
+static int rpc_spi_remove(struct platform_device *pdev)
+{
+ struct spi_controller *ctlr = platform_get_drvdata(pdev);
+
+ pm_runtime_disable(&pdev->dev);
+ spi_unregister_controller(ctlr);
+
+ return 0;
+}
+
+static const struct of_device_id rpc_spi_of_ids[] = {
+ { .compatible = "renesas,rcar-rpc-spi", },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rpc_spi_of_ids);
+
+#ifdef CONFIG_PM_SLEEP
+static int rpc_spi_suspend(struct device *dev)
+{
+ struct spi_controller *ctlr = dev_get_drvdata(dev);
+
+ return spi_controller_suspend(ctlr);
+}
+
+static int rpc_spi_resume(struct device *dev)
+{
+ struct spi_controller *ctlr = dev_get_drvdata(dev);
+
+ return spi_controller_resume(ctlr);
+}
+
+static SIMPLE_DEV_PM_OPS(rpc_spi_pm_ops, rpc_spi_suspend, rpc_spi_resume);
+#define DEV_PM_OPS (&rpc_spi_pm_ops)
+#else
+#define DEV_PM_OPS NULL
+#endif
+
+static struct platform_driver rpc_spi_driver = {
+ .probe = rpc_spi_probe,
+ .remove = rpc_spi_remove,
+ .driver = {
+ .name = "rpc-spi",
+ .of_match_table = rpc_spi_of_ids,
+ .pm = DEV_PM_OPS,
+ },
+};
+module_platform_driver(rpc_spi_driver);
+
+MODULE_AUTHOR("Mason Yang <[email protected]>");
+MODULE_DESCRIPTION("Renesas R-Car Gen3 RPC-IF SPI controller driver");
+MODULE_LICENSE("GPL v2");
--
1.9.1


2019-03-29 08:14:48

by Mason Yang

[permalink] [raw]
Subject: [PATCH v9 1/3] mfd: Add Renesas R-Car Gen3 RPC-IF MFD controller driver

Add a driver for Renesas R-Car Gen3 RPC-IF MFD controller.

Signed-off-by: Mason Yang <[email protected]>
---
drivers/mfd/Kconfig | 9 +++
drivers/mfd/Makefile | 1 +
drivers/mfd/renesas-rpc.c | 140 ++++++++++++++++++++++++++++++++++++
include/linux/mfd/renesas-rpc.h | 154 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 304 insertions(+)
create mode 100644 drivers/mfd/renesas-rpc.c
create mode 100644 include/linux/mfd/renesas-rpc.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 0ce2d8d..a870e12 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -978,6 +978,15 @@ config MFD_RDC321X
southbridge which provides access to GPIOs and Watchdog using the
southbridge PCI device configuration space.

+config MFD_RENESAS_RPC
+ tristate "Renesas R-Car Gen3 RPC-IF MFD driver"
+ select MFD_CORE
+ depends on ARCH_RENESAS
+ help
+ This supports for Renesas R-Car Gen3 RPC-IF multifunction device
+ controller which provides either SPI host controller or HyperFlash.
+ You have to select individual components under the corresponding menu.
+
config MFD_RT5033
tristate "Richtek RT5033 Power Management IC"
depends on I2C
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index b4569ed7..4a49699 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -183,6 +183,7 @@ obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO) += intel_quark_i2c_gpio.o
obj-$(CONFIG_LPC_SCH) += lpc_sch.o
obj-$(CONFIG_LPC_ICH) += lpc_ich.o
obj-$(CONFIG_MFD_RDC321X) += rdc321x-southbridge.o
+obj-$(CONFIG_MFD_RENESAS_RPC) += renesas-rpc.o
obj-$(CONFIG_MFD_JANZ_CMODIO) += janz-cmodio.o
obj-$(CONFIG_MFD_JZ4740_ADC) += jz4740-adc.o
obj-$(CONFIG_MFD_TPS6586X) += tps6586x.o
diff --git a/drivers/mfd/renesas-rpc.c b/drivers/mfd/renesas-rpc.c
new file mode 100644
index 0000000..c92bb74
--- /dev/null
+++ b/drivers/mfd/renesas-rpc.c
@@ -0,0 +1,140 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (C) 2018 ~ 2019 Renesas Solutions Corp.
+// Copyright (C) 2019 Macronix International Co., Ltd.
+//
+// R-Car Gen3 RPC-IF MFD driver
+//
+// Author:
+// Mason Yang <[email protected]>
+//
+
+#include <linux/mfd/renesas-rpc.h>
+#include <linux/mfd/core.h>
+
+static const struct mfd_cell rpc_hf_ctlr = {
+ .name = "rpc-hf",
+ .of_compatible = "renesas,rcar-rpc-hf",
+};
+
+static const struct mfd_cell rpc_spi_ctlr = {
+ .name = "rpc-spi",
+ .of_compatible = "renesas,rcar-rpc-spi",
+};
+
+static const struct regmap_range rpc_mfd_volatile_ranges[] = {
+ regmap_reg_range(RPC_SMRDR0, RPC_SMRDR0),
+ regmap_reg_range(RPC_SMWDR0, RPC_SMWDR0),
+ regmap_reg_range(RPC_CMNSR, RPC_CMNSR),
+};
+
+static const struct regmap_access_table rpc_mfd_volatile_table = {
+ .yes_ranges = rpc_mfd_volatile_ranges,
+ .n_yes_ranges = ARRAY_SIZE(rpc_mfd_volatile_ranges),
+};
+
+static const struct regmap_config rpc_mfd_regmap_config = {
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = 4,
+ .fast_io = true,
+ .max_register = RPC_PHYOFFSET2,
+ .volatile_table = &rpc_mfd_volatile_table,
+};
+
+static int rpc_mfd_probe(struct platform_device *pdev)
+{
+ struct device_node *ctlr;
+ const struct mfd_cell *cell;
+ struct resource *res;
+ struct rpc_mfd *rpc;
+ int ret;
+
+ ctlr = of_get_next_child(pdev->dev.of_node, NULL);
+ if (!ctlr) {
+ dev_warn(&pdev->dev, "no spi/hf ctlr node found\n");
+ return -ENODEV;
+ }
+
+ ret = of_device_is_compatible(ctlr, "renesas,rcar-rpc-spi");
+ if (ret) {
+ cell = &rpc_spi_ctlr;
+ } else {
+ ret = of_device_is_compatible(ctlr,
+ "renesas,rcar-rpc-hf");
+ if (ret) {
+ cell = &rpc_hf_ctlr;
+ } else {
+ dev_warn(&pdev->dev, "no spi/hf device found\n");
+ return -ENODEV;
+ }
+ }
+
+ rpc = devm_kzalloc(&pdev->dev, sizeof(*rpc), GFP_KERNEL);
+ if (!rpc)
+ return -ENOMEM;
+
+ rpc->clk_rpc = devm_clk_get(&pdev->dev, "rpc");
+ if (IS_ERR(rpc->clk_rpc))
+ return PTR_ERR(rpc->clk_rpc);
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regs");
+ rpc->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(rpc->base))
+ return PTR_ERR(rpc->base);
+
+ rpc->regmap = devm_regmap_init_mmio(&pdev->dev, rpc->base,
+ &rpc_mfd_regmap_config);
+ if (IS_ERR(rpc->regmap)) {
+ dev_err(&pdev->dev,
+ "failed to init regmap for rpc-mfd, error %ld\n",
+ PTR_ERR(rpc->regmap));
+ return PTR_ERR(rpc->regmap);
+ }
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dirmap");
+ rpc->dirmap = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(rpc->dirmap))
+ rpc->dirmap = NULL;
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "wbuf");
+ rpc->wbuf = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(rpc->wbuf))
+ rpc->wbuf = NULL;
+
+ rpc->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+ if (IS_ERR(rpc->rstc))
+ return PTR_ERR(rpc->rstc);
+
+ platform_set_drvdata(pdev, rpc);
+
+ ret = devm_mfd_add_devices(&pdev->dev, -1, cell, 1, NULL, 0, NULL);
+
+ return ret;
+}
+
+static int rpc_mfd_remove(struct platform_device *pdev)
+{
+ mfd_remove_devices(&pdev->dev);
+ return 0;
+}
+
+static const struct of_device_id rpc_mfd_of_match[] = {
+ { .compatible = "renesas,rcar-gen3-rpc", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, rpc_mfd_of_match);
+
+static struct platform_driver rpc_mfd_driver = {
+ .probe = rpc_mfd_probe,
+ .remove = rpc_mfd_remove,
+ .driver = {
+ .name = "rpc-mfd",
+ .of_match_table = rpc_mfd_of_match,
+ },
+};
+module_platform_driver(rpc_mfd_driver);
+
+MODULE_AUTHOR("Mason Yang <[email protected]>");
+MODULE_DESCRIPTION("Renesas R-Car Gen3 RPC MFD driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/renesas-rpc.h b/include/linux/mfd/renesas-rpc.h
new file mode 100644
index 0000000..209e64f1
--- /dev/null
+++ b/include/linux/mfd/renesas-rpc.h
@@ -0,0 +1,154 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (C) 2018 ~ 2019 Renesas Solutions Corp.
+// Copyright (C) 2019 Macronix International Co., Ltd.
+//
+// R-Car Gen3 RPC-IF MFD driver
+//
+// Author:
+// Mason Yang <[email protected]>
+//
+
+#ifndef __MFD_RENESAS_RPC_H
+#define __MFD_RENESAS_RPC_H
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/log2.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+#define RPC_CMNCR 0x0000 // R/W
+#define RPC_CMNCR_MD BIT(31)
+#define RPC_CMNCR_SFDE BIT(24) // undocumented bit but must be set
+#define RPC_CMNCR_MOIIO3(val) (((val) & 0x3) << 22)
+#define RPC_CMNCR_MOIIO2(val) (((val) & 0x3) << 20)
+#define RPC_CMNCR_MOIIO1(val) (((val) & 0x3) << 18)
+#define RPC_CMNCR_MOIIO0(val) (((val) & 0x3) << 16)
+#define RPC_CMNCR_MOIIO_HIZ (RPC_CMNCR_MOIIO0(3) | RPC_CMNCR_MOIIO1(3) | \
+ RPC_CMNCR_MOIIO2(3) | RPC_CMNCR_MOIIO3(3))
+#define RPC_CMNCR_IO3FV(val) (((val) & 0x3) << 14) // undocumented
+#define RPC_CMNCR_IO2FV(val) (((val) & 0x3) << 12) // undocumented
+#define RPC_CMNCR_IO0FV(val) (((val) & 0x3) << 8)
+#define RPC_CMNCR_IOFV_HIZ (RPC_CMNCR_IO0FV(3) | RPC_CMNCR_IO2FV(3) | \
+ RPC_CMNCR_IO3FV(3))
+#define RPC_CMNCR_BSZ(val) (((val) & 0x3) << 0)
+
+#define RPC_SSLDR 0x0004 // R/W
+#define RPC_SSLDR_SPNDL(d) (((d) & 0x7) << 16)
+#define RPC_SSLDR_SLNDL(d) (((d) & 0x7) << 8)
+#define RPC_SSLDR_SCKDL(d) (((d) & 0x7) << 0)
+
+#define RPC_DRCR 0x000C // R/W
+#define RPC_DRCR_SSLN BIT(24)
+#define RPC_DRCR_RBURST(v) ((((v) - 1) & 0x1F) << 16)
+#define RPC_DRCR_RCF BIT(9)
+#define RPC_DRCR_RBE BIT(8)
+#define RPC_DRCR_SSLE BIT(0)
+
+#define RPC_DRCMR 0x0010 // R/W
+#define RPC_DRCMR_CMD(c) (((c) & 0xFF) << 16)
+#define RPC_DRCMR_OCMD(c) (((c) & 0xFF) << 0)
+
+#define RPC_DREAR 0x0014 // R/W
+#define RPC_DREAR_EAC(c) (((c) & 0x7) << 0)
+
+#define RPC_DROPR 0x0018 // R/W
+
+#define RPC_DRENR 0x001C // R/W
+#define RPC_DRENR_CDB(o) (u32)((((o) & 0x3) << 30))
+#define RPC_DRENR_OCDB(o) (((o) & 0x3) << 28)
+#define RPC_DRENR_ADB(o) (((o) & 0x3) << 24)
+#define RPC_DRENR_OPDB(o) (((o) & 0x3) << 20)
+#define RPC_DRENR_DRDB(o) (((o) & 0x3) << 16)
+#define RPC_DRENR_DME BIT(15)
+#define RPC_DRENR_CDE BIT(14)
+#define RPC_DRENR_OCDE BIT(12)
+#define RPC_DRENR_ADE(v) (((v) & 0xF) << 8)
+#define RPC_DRENR_OPDE(v) (((v) & 0xF) << 4)
+
+#define RPC_SMCR 0x0020 // R/W
+#define RPC_SMCR_SSLKP BIT(8)
+#define RPC_SMCR_SPIRE BIT(2)
+#define RPC_SMCR_SPIWE BIT(1)
+#define RPC_SMCR_SPIE BIT(0)
+
+#define RPC_SMCMR 0x0024 // R/W
+#define RPC_SMCMR_CMD(c) (((c) & 0xFF) << 16)
+#define RPC_SMCMR_OCMD(c) (((c) & 0xFF) << 0)
+
+#define RPC_SMADR 0x0028 // R/W
+#define RPC_SMOPR 0x002C // R/W
+#define RPC_SMOPR_OPD3(o) (((o) & 0xFF) << 24)
+#define RPC_SMOPR_OPD2(o) (((o) & 0xFF) << 16)
+#define RPC_SMOPR_OPD1(o) (((o) & 0xFF) << 8)
+#define RPC_SMOPR_OPD0(o) (((o) & 0xFF) << 0)
+
+#define RPC_SMENR 0x0030 // R/W
+#define RPC_SMENR_CDB(o) (((o) & 0x3) << 30)
+#define RPC_SMENR_OCDB(o) (((o) & 0x3) << 28)
+#define RPC_SMENR_ADB(o) (((o) & 0x3) << 24)
+#define RPC_SMENR_OPDB(o) (((o) & 0x3) << 20)
+#define RPC_SMENR_SPIDB(o) (((o) & 0x3) << 16)
+#define RPC_SMENR_DME BIT(15)
+#define RPC_SMENR_CDE BIT(14)
+#define RPC_SMENR_OCDE BIT(12)
+#define RPC_SMENR_ADE(v) (((v) & 0xF) << 8)
+#define RPC_SMENR_OPDE(v) (((v) & 0xF) << 4)
+#define RPC_SMENR_SPIDE(v) (((v) & 0xF) << 0)
+
+#define RPC_SMRDR0 0x0038 // R
+#define RPC_SMRDR1 0x003C // R
+#define RPC_SMWDR0 0x0040 // W
+#define RPC_SMWDR1 0x0044 // W
+
+#define RPC_CMNSR 0x0048 // R
+#define RPC_CMNSR_SSLF BIT(1)
+#define RPC_CMNSR_TEND BIT(0)
+
+#define RPC_DRDMCR 0x0058 // R/W
+#define RPC_DRDRENR 0x005C // R/W
+
+#define RPC_SMDMCR 0x0060 // R/W
+#define RPC_SMDMCR_DMCYC(v) ((((v) - 1) & 0x1F) << 0)
+
+#define RPC_SMDRENR 0x0064 // R/W
+#define RPC_SMDRENR_HYPE (0x7 << 12)
+#define RPC_SMDRENR_ADDRE BIT(8)
+#define RPC_SMDRENR_OPDRE BIT(4)
+#define RPC_SMDRENR_SPIDRE BIT(0)
+
+#define RPC_PHYCNT 0x007C // R/W
+#define RPC_PHYCNT_CAL BIT(31)
+#define PRC_PHYCNT_OCTA_AA BIT(22)
+#define PRC_PHYCNT_OCTA_SA BIT(23)
+#define PRC_PHYCNT_EXDS BIT(21)
+#define RPC_PHYCNT_OCT BIT(20)
+#define RPC_PHYCNT_STRTIM(v) (((v) & 0x7) << 15)
+#define RPC_PHYCNT_WBUF2 BIT(4)
+#define RPC_PHYCNT_WBUF BIT(2)
+#define RPC_PHYCNT_PHYMEM(v) (((v) & 0x3) << 0)
+
+#define RPC_PHYOFFSET1 0x0080 // R/W
+#define RPC_PHYOFFSET1_DDRTMG(v) (((v) & 0x3) << 28)
+#define RPC_PHYOFFSET2 0x0084 // R/W
+#define RPC_PHYOFFSET2_OCTTMG(v) (((v) & 0x7) << 8)
+
+#define RPC_WBUF_SIZE 256 // Write Buffer size
+
+struct rpc_mfd {
+ struct clk *clk_rpc;
+ void __iomem *base;
+ void __iomem *dirmap;
+ void __iomem *wbuf;
+ struct regmap *regmap;
+ struct reset_control *rstc;
+};
+
+#endif // __MFD_RENESAS_RPC_H
--
1.9.1


2019-03-29 10:47:27

by Marek Vasut

[permalink] [raw]
Subject: Re: [PATCH v9 1/3] mfd: Add Renesas R-Car Gen3 RPC-IF MFD controller driver

On 3/29/19 9:20 AM, Mason Yang wrote:
> Add a driver for Renesas R-Car Gen3 RPC-IF MFD controller.
>
> Signed-off-by: Mason Yang <[email protected]>
> ---
> drivers/mfd/Kconfig | 9 +++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/renesas-rpc.c | 140 ++++++++++++++++++++++++++++++++++++
> include/linux/mfd/renesas-rpc.h | 154 ++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 304 insertions(+)
> create mode 100644 drivers/mfd/renesas-rpc.c
> create mode 100644 include/linux/mfd/renesas-rpc.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 0ce2d8d..a870e12 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -978,6 +978,15 @@ config MFD_RDC321X
> southbridge which provides access to GPIOs and Watchdog using the
> southbridge PCI device configuration space.
>
> +config MFD_RENESAS_RPC
> + tristate "Renesas R-Car Gen3 RPC-IF MFD driver"
> + select MFD_CORE
> + depends on ARCH_RENESAS
> + help
> + This supports for Renesas R-Car Gen3 RPC-IF multifunction device
> + controller which provides either SPI host controller or HyperFlash.
> + You have to select individual components under the corresponding menu.
> +
> config MFD_RT5033
> tristate "Richtek RT5033 Power Management IC"
> depends on I2C
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index b4569ed7..4a49699 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -183,6 +183,7 @@ obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO) += intel_quark_i2c_gpio.o
> obj-$(CONFIG_LPC_SCH) += lpc_sch.o
> obj-$(CONFIG_LPC_ICH) += lpc_ich.o
> obj-$(CONFIG_MFD_RDC321X) += rdc321x-southbridge.o
> +obj-$(CONFIG_MFD_RENESAS_RPC) += renesas-rpc.o
> obj-$(CONFIG_MFD_JANZ_CMODIO) += janz-cmodio.o
> obj-$(CONFIG_MFD_JZ4740_ADC) += jz4740-adc.o
> obj-$(CONFIG_MFD_TPS6586X) += tps6586x.o
> diff --git a/drivers/mfd/renesas-rpc.c b/drivers/mfd/renesas-rpc.c
> new file mode 100644
> index 0000000..c92bb74
> --- /dev/null
> +++ b/drivers/mfd/renesas-rpc.c
> @@ -0,0 +1,140 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (C) 2018 ~ 2019 Renesas Solutions Corp.
> +// Copyright (C) 2019 Macronix International Co., Ltd.
> +//
> +// R-Car Gen3 RPC-IF MFD driver
> +//
> +// Author:
> +// Mason Yang <[email protected]>
> +//
> +
> +#include <linux/mfd/renesas-rpc.h>
> +#include <linux/mfd/core.h>
> +
> +static const struct mfd_cell rpc_hf_ctlr = {
> + .name = "rpc-hf",
> + .of_compatible = "renesas,rcar-rpc-hf",
> +};
> +
> +static const struct mfd_cell rpc_spi_ctlr = {
> + .name = "rpc-spi",
> + .of_compatible = "renesas,rcar-rpc-spi",
> +};
> +
> +static const struct regmap_range rpc_mfd_volatile_ranges[] = {
> + regmap_reg_range(RPC_SMRDR0, RPC_SMRDR0),
> + regmap_reg_range(RPC_SMWDR0, RPC_SMWDR0),
> + regmap_reg_range(RPC_CMNSR, RPC_CMNSR),
> +};

Isn't SMWDR1 volatile too ? And SMRDR1 too ?

[...]
--
Best regards,
Marek Vasut

2019-03-29 10:48:15

by Marek Vasut

[permalink] [raw]
Subject: Re: [PATCH v9 3/3] dt-bindings: mfd: Document Renesas R-Car Gen3 RPC-IF controller bindings

On 3/29/19 9:20 AM, Mason Yang wrote:
> Document the bindings used by the Renesas R-Car Gen3 RPC-IF MFD controller.
>
> Signed-off-by: Mason Yang <[email protected]>
> ---
> .../devicetree/bindings/mfd/mfd-renesas-rpc.txt | 57 ++++++++++++++++++++++
> 1 file changed, 57 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
>
> diff --git a/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
> new file mode 100644
> index 0000000..577986b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
> @@ -0,0 +1,57 @@
> +Renesas R-Car Gen3 RPC-IF MFD controller Device Tree Bindings
> +---------------------------------------------------------------------
> +
> +Required properties:
> +- compatible: should be an SoC-specific compatible value, followed by
> + "renesas,rcar-gen3-rpc" as a fallback.
> + supported SoC-specific values are:
> + "renesas,r8a77995-rpc" (R-Car D3)
> +- reg: should contain three register areas:
> + first for the base address of rpc-if registers,
> + second for the direct mapping read mode and
> + third for the write buffer area.
> +- reg-names: should contain "regs", "dirmap" and "wbuf"
> +- clocks: should contain 1 entries for the module's clock
> +- clock-names: should contain "rpc"
> +- #address-cells: should be 1
> +- #size-cells: should be 0
> +
> +Required nodes:
> + spi:
> + Node for configuring the SPI controller driver.
> + Required properties:
> + compatible = "renesas,rcar-rpc-spi";
> +
> + hf:
> + Node for configuring the hyperflash controller driver.
> + Required properties:
> + compatible = "renesas,rcar-rpc-hf";
> +
> +Example:
> +
> + rpc_mfd: rpc-mfd@ee200000 {
> + compatible = "renesas,r8a77995-rpc", "renesas,rcar-gen3-rpc";
> + reg = <0 0xee200000 0 0x200>, <0 0x08000000 0 0x4000000>,
> + <0 0xee208000 0 0x100>;
> + reg-names = "regs", "dirmap", "wbuf";
> + clocks = <&cpg CPG_MOD 917>;
> + clock-names = "rpc";
> + power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;
> + resets = <&cpg 917>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + spi {

Why is this subnode needed ?
The MFD driver core can just check whether the flash@0 is jedec,spi-nor
or cfi-flash and select mode of operation based on that.

> + compatible = "renesas,rcar-rpc-spi";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + flash@0 {
> + compatible = "jedec,spi-nor";
> + reg = <0>;
> + spi-max-frequency = <40000000>;
> + spi-tx-bus-width = <1>;
> + spi-rx-bus-width = <1>;
> + };
> + };
> + };
>


--
Best regards,
Marek Vasut

2019-03-29 10:48:50

by Marek Vasut

[permalink] [raw]
Subject: Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver

On 3/29/19 9:20 AM, Mason Yang wrote:

[...]

> +struct rpc_spi {
> + struct clk *clk_rpc;
> + void __iomem *base;
> + void __iomem *dirmap;
> + void __iomem *wbuf;

Shouldn't all this stuff somehow come from the MFD driver ?

> + struct regmap *regmap;
> + u32 cur_speed_hz;
> + u32 cmd;
> + u32 addr;
> + u32 dummy;
> + u32 smcr;
> + u32 smenr;
> + u32 xferlen;
> + u32 totalxferlen;
> + enum spi_mem_data_dir xfer_dir;
> + struct reset_control *rstc;
> +};
> +
[...]


--
Best regards,
Marek Vasut

2019-03-29 10:51:00

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH v9 3/3] dt-bindings: mfd: Document Renesas R-Car Gen3 RPC-IF controller bindings

Hello!

On 03/29/2019 11:20 AM, Mason Yang wrote:

> Document the bindings used by the Renesas R-Car Gen3 RPC-IF MFD controller.
>
> Signed-off-by: Mason Yang <[email protected]>
> ---
> .../devicetree/bindings/mfd/mfd-renesas-rpc.txt | 57 ++++++++++++++++++++++
> 1 file changed, 57 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
>
> diff --git a/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
> new file mode 100644
> index 0000000..577986b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
> @@ -0,0 +1,57 @@
> +Renesas R-Car Gen3 RPC-IF MFD controller Device Tree Bindings
> +---------------------------------------------------------------------

Too many dashes. :-)

> +
> +Required properties:
> +- compatible: should be an SoC-specific compatible value, followed by
> + "renesas,rcar-gen3-rpc" as a fallback.
> + supported SoC-specific values are:
> + "renesas,r8a77995-rpc" (R-Car D3)
> +- reg: should contain three register areas:
> + first for the base address of rpc-if registers,
> + second for the direct mapping read mode and
> + third for the write buffer area.
> +- reg-names: should contain "regs", "dirmap" and "wbuf"
> +- clocks: should contain 1 entries for the module's clock
> +- clock-names: should contain "rpc"

What about the "power-domains" and "resets" props?

> +- #address-cells: should be 1
> +- #size-cells: should be 0

Why? You don't have the "reg" props in the sub-devices...

> +
> +Required nodes:
> + spi:
> + Node for configuring the SPI controller driver.
> + Required properties:
> + compatible = "renesas,rcar-rpc-spi";

Sigh... you were supposed to only place the flash device sub-nodes
under the "main" device node, we don't add the virtual devices to DT,
there's no matching sub-device in the hardware. You don't have to
describe these sub-devices in DT, you can register the simple platform
devices.

> +
> + hf:
> + Node for configuring the hyperflash controller driver.
> + Required properties:
> + compatible = "renesas,rcar-rpc-hf";
> +
> +Example:
> +
> + rpc_mfd: rpc-mfd@ee200000 {

Just "rpc: rpc-if@ee200000, please.

[...]

MBR, Sergei

2019-03-29 15:13:44

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH v9 1/3] mfd: Add Renesas R-Car Gen3 RPC-IF MFD controller driver

On 03/29/2019 11:20 AM, Mason Yang wrote:

> Add a driver for Renesas R-Car Gen3 RPC-IF MFD controller.
>
> Signed-off-by: Mason Yang <[email protected]>
[...]
> diff --git a/drivers/mfd/renesas-rpc.c b/drivers/mfd/renesas-rpc.c
> new file mode 100644
> index 0000000..c92bb74
> --- /dev/null
> +++ b/drivers/mfd/renesas-rpc.c
> @@ -0,0 +1,140 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (C) 2018 ~ 2019 Renesas Solutions Corp.
> +// Copyright (C) 2019 Macronix International Co., Ltd.
> +//
> +// R-Car Gen3 RPC-IF MFD driver
> +//
> +// Author:
> +// Mason Yang <[email protected]>
> +//
> +
> +#include <linux/mfd/renesas-rpc.h>
> +#include <linux/mfd/core.h>
> +
> +static const struct mfd_cell rpc_hf_ctlr = {
> + .name = "rpc-hf",
> + .of_compatible = "renesas,rcar-rpc-hf",
> +};
> +
> +static const struct mfd_cell rpc_spi_ctlr = {
> + .name = "rpc-spi",
> + .of_compatible = "renesas,rcar-rpc-spi",
> +};
> +
> +static const struct regmap_range rpc_mfd_volatile_ranges[] = {
> + regmap_reg_range(RPC_SMRDR0, RPC_SMRDR0),

regmap_reg_range((RPC_SMRDR0, RPC_SMRDR1), like Marek noted?

> + regmap_reg_range(RPC_SMWDR0, RPC_SMWDR0),

regmap_reg_range((RPC_SMWDR0, RPC_SMWDR1)?

> +static int rpc_mfd_probe(struct platform_device *pdev)
> +{
> + struct device_node *ctlr;
> + const struct mfd_cell *cell;
> + struct resource *res;
> + struct rpc_mfd *rpc;
> + int ret;
> +
> + ctlr = of_get_next_child(pdev->dev.of_node, NULL);

The child should be a CFI or JEDEC flash device, no virtual devices,
please.

[...]
> + rpc->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> + if (IS_ERR(rpc->rstc))
> + return PTR_ERR(rpc->rstc);
> +
> + platform_set_drvdata(pdev, rpc);
> +
> + ret = devm_mfd_add_devices(&pdev->dev, -1, cell, 1, NULL, 0, NULL);
> +
> + return ret;

return devm_mfd_add_devices(&pdev->dev, -1, cell, 1, NULL, 0, NULL);

> +}
> +
> +static int rpc_mfd_remove(struct platform_device *pdev)
> +{
> + mfd_remove_devices(&pdev->dev);

Why, if you used devm_mfd_add_devices() in the probe method?

> + return 0;
> +}
[...]
> diff --git a/include/linux/mfd/renesas-rpc.h b/include/linux/mfd/renesas-rpc.h
> new file mode 100644
> index 0000000..209e64f1
> --- /dev/null
> +++ b/include/linux/mfd/renesas-rpc.h
> @@ -0,0 +1,154 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (C) 2018 ~ 2019 Renesas Solutions Corp.
> +// Copyright (C) 2019 Macronix International Co., Ltd.
> +//
> +// R-Car Gen3 RPC-IF MFD driver
> +//
> +// Author:
> +// Mason Yang <[email protected]>
> +//
> +
> +#ifndef __MFD_RENESAS_RPC_H
> +#define __MFD_RENESAS_RPC_H
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/log2.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>

Is any of these #include's used by the header itself? I don't think so...

> +
> +#define RPC_CMNCR 0x0000 // R/W

Not sure we'd need the registers described in the public header after
we place the common RPC-IF code into the MFD driver. We'll see though...

[...]

MBR, Sergei

2019-03-29 15:54:13

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver

On 03/29/2019 11:20 AM, Mason Yang wrote:

> Add a driver for Renesas R-Car Gen3 RPC-IF SPI controller.
>
> Signed-off-by: Mason Yang <[email protected]>
> Signed-off-by: Sergei Shtylyov <[email protected]>
[...]
> diff --git a/drivers/spi/spi-renesas-rpc.c b/drivers/spi/spi-renesas-rpc.c
> new file mode 100644
> index 0000000..037f273
> --- /dev/null
> +++ b/drivers/spi/spi-renesas-rpc.c
> @@ -0,0 +1,640 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (C) 2018 ~ 2019 Renesas Solutions Corp.
> +// Copyright (C) 2019 Macronix International Co., Ltd.
> +//
> +// R-Car Gen3 RPC-IF SPI/QSPI/Octa driver
> +//
> +// Author:
> +// Mason Yang <[email protected]>
> +//
> +
> +#include <linux/mfd/renesas-rpc.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spi-mem.h>
> +
> +#include <asm/unaligned.h>
> +
> +struct rpc_spi {
> + struct clk *clk_rpc;
> + void __iomem *base;
> + void __iomem *dirmap;
> + void __iomem *wbuf;
> + struct regmap *regmap;

Why copy struct rpc_mfd's fields?

> + u32 cur_speed_hz;
> + u32 cmd;
> + u32 addr;
> + u32 dummy;
> + u32 smcr;
> + u32 smenr;

I suggest renaming this field to 'enable'.

> + u32 xferlen;
> + u32 totalxferlen;
> + enum spi_mem_data_dir xfer_dir;
> + struct reset_control *rstc;
> +};
[...]
> +static void rpc_spi_hw_init(struct rpc_spi *rpc)
> +{
> + //
> + // NOTE: The 0x260 are undocumented bits, but they must be set.
> + // RPC_PHYCNT_STRTIM is strobe timing adjustment bit,
> + // 0x0 : the delay is biggest,
> + // 0x1 : the delay is 2nd biggest,
> + // On H3 ES1.x, the value should be 0, while on others,
> + // the value should be 6.
> + //
> + regmap_write(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_CAL |
> + RPC_PHYCNT_STRTIM(6) | 0x260);
> +
> + //
> + // NOTE: The 0x1511144 are undocumented bits, but they must be set
> + // for RPC_PHYOFFSET1.
> + // The 0x31 are undocumented bits, but they must be set
> + // for RPC_PHYOFFSET2.
> + //
> + regmap_write(rpc->regmap, RPC_PHYOFFSET1, RPC_PHYOFFSET1_DDRTMG(3) |
> + 0x1511144);
> + regmap_write(rpc->regmap, RPC_PHYOFFSET2, 0x31 |
> + RPC_PHYOFFSET2_OCTTMG(4));
> + regmap_write(rpc->regmap, RPC_SSLDR, RPC_SSLDR_SPNDL(7) |
> + RPC_SSLDR_SLNDL(7) | RPC_SSLDR_SCKDL(7));
> + regmap_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD | RPC_CMNCR_SFDE |
> + RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
> + RPC_CMNCR_BSZ(0));
> +}

This function looks like a candidate to moving to the common (MFD) driver.

> +
> +static int wait_msg_xfer_end(struct rpc_spi *rpc)
> +{
> + u32 sts;
> +
> + return regmap_read_poll_timeout(rpc->regmap, RPC_CMNSR, sts,
> + sts & RPC_CMNSR_TEND, 0, USEC_PER_SEC);
> +}

This as well.

> +static void rpc_spi_mem_set_prep_op_cfg(struct spi_device *spi,
> + const struct spi_mem_op *op,
> + u64 *offs, size_t *len)
> +{
> + struct rpc_spi *rpc = spi_controller_get_devdata(spi->controller);
> +
> + rpc->cmd = RPC_SMCMR_CMD(op->cmd.opcode);
> + rpc->smenr = RPC_SMENR_CDE |
> + RPC_SMENR_CDB(ilog2(op->cmd.buswidth));
> + rpc->totalxferlen = 1;
> + rpc->xfer_dir = SPI_MEM_NO_DATA;
> + rpc->xferlen = 0;
> + rpc->addr = 0;
> +
> + if (op->addr.nbytes) {
> + rpc->smenr |= RPC_SMENR_ADB(ilog2(op->addr.buswidth));
> + if (op->addr.nbytes == 4)
> + rpc->smenr |= RPC_SMENR_ADE(0xf);
> + else
> + rpc->smenr |= RPC_SMENR_ADE(0x7);
> +
> + if (offs && len)
> + rpc->addr = *offs;
> + else
> + rpc->addr = op->addr.val;
> + rpc->totalxferlen += op->addr.nbytes;
> + }
> +
> + if (op->dummy.nbytes) {
> + rpc->smenr |= RPC_SMENR_DME;
> + rpc->dummy = RPC_SMDMCR_DMCYC(op->dummy.nbytes);

So you haven't fixed this bug? I repeat, the driver doesn't work right
w/o this fixed!

[...]
> +static ssize_t rpc_spi_mem_dirmap_read(struct spi_mem_dirmap_desc *desc,
> + u64 offs, size_t len, void *buf)
> +{
> + struct rpc_spi *rpc =
> + spi_controller_get_devdata(desc->mem->spi->controller);
> + int ret;
> +
> + if (offs + desc->info.offset + len > U32_MAX)
> + return -EINVAL;
> +
> + if (len > 0x4000000)
> + len = 0x4000000;

Ugh...

> +
> + ret = rpc_spi_set_freq(rpc, desc->mem->spi->max_speed_hz);
> + if (ret)
> + return ret;
> +
> + rpc_spi_mem_set_prep_op_cfg(desc->mem->spi,
> + &desc->info.op_tmpl, &offs, &len);
> +
> + regmap_update_bits(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD, 0);
> + regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RBURST(32) |
> + RPC_DRCR_RBE);
> +
> + regmap_write(rpc->regmap, RPC_DRCMR, rpc->cmd);
> + regmap_write(rpc->regmap, RPC_DREAR, RPC_DREAR_EAC(1));

So you're not even trying to support flashes larger than the read dirmap?
Now I don't think it's acceptable (and I have rewritten this code internally).

[...]
> +static ssize_t rpc_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
> + u64 offs, size_t len, const void *buf)
> +{
> + struct rpc_spi *rpc =
> + spi_controller_get_devdata(desc->mem->spi->controller);
> + int ret;
> +
> + if (offs + desc->info.offset + len > U32_MAX)
> + return -EINVAL;
> +
> + if (len > RPC_WBUF_SIZE)
> + len = RPC_WBUF_SIZE;

Ugh! Again, I no longer think this is acceptable... Maybe we just need
to drop the support of the write buffer...

[...]
> +static void rpc_spi_transfer_setup(struct rpc_spi *rpc,
> + struct spi_message *msg)
> +{
[...]
> + if (xfercnt > 2 && xfer[1].len && xfer[1].tx_buf) {
> + rpc->smenr |=
> + RPC_SMENR_ADB(ilog2((unsigned int)xfer[1].tx_nbits));
> +
> + for (i = 0; i < xfer[1].len; i++)
> + rpc->addr |= ((u8 *)xfer[1].tx_buf)[i] <<
> + (8 * (xfer[1].len - i - 1));
> +
> + if (xfer[1].len == 4)
> + rpc->smenr |= RPC_SMENR_ADE(0xf);
> + else
> + rpc->smenr |= RPC_SMENR_ADE(0x7);
> + }
> +
> + if (xfercnt > 3 && xfer[2].len && xfer[2].tx_buf) {
> + rpc->smenr |= RPC_SMENR_DME;
> + rpc->dummy = RPC_SMDMCR_DMCYC(xfer[2].len);

Needs to be multiplied by 8 (or other value) as well...

[...]
> +static int rpc_spi_probe(struct platform_device *pdev)
> +{
> + struct spi_controller *ctlr;
> + struct rpc_mfd *rpc_mfd = dev_get_drvdata(pdev->dev.parent);
> + struct rpc_spi *rpc;
> + int ret;
[...]
> + rpc->clk_rpc = rpc_mfd->clk_rpc;
> + if (IS_ERR(rpc->clk_rpc))
> + return PTR_ERR(rpc->clk_rpc);
> +
> + rpc->base = rpc_mfd->base;
> + if (IS_ERR(rpc->base))
> + return PTR_ERR(rpc->base);
> +
> + rpc->regmap = rpc_mfd->regmap;
> + if (IS_ERR(rpc->regmap)) {
> + dev_err(&pdev->dev,
> + "failed to regmap for rpc-spi, error %ld\n",
> + PTR_ERR(rpc->regmap));
> + return PTR_ERR(rpc->regmap);
> + }
> +
> + rpc->dirmap = rpc_mfd->dirmap;
> + if (IS_ERR(rpc->dirmap))
> + rpc->dirmap = NULL;
> +
> + rpc->wbuf = rpc_mfd->wbuf;
> + if (IS_ERR(rpc->wbuf))
> + rpc->wbuf = NULL;
> +
> + rpc->rstc = rpc_mfd->rstc;
> + if (IS_ERR(rpc->rstc))
> + return PTR_ERR(rpc->rstc);

Why do we need all this copying?

> +
> + pm_runtime_enable(&pdev->dev);
> + ctlr->auto_runtime_pm = true;
> +
> + ctlr->num_chipselect = 1;
> + ctlr->mem_ops = &rpc_spi_mem_ops;
> + ctlr->transfer_one_message = rpc_spi_transfer_one_message;
> +
> + ctlr->bits_per_word_mask = SPI_BPW_MASK(8);
> + ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_TX_QUAD | SPI_RX_QUAD;
> +

We do need pm_runtime_get_sync() here!

> + rpc_spi_hw_init(rpc);

And pm_runtime_put() here...

[...]
> +static const struct of_device_id rpc_spi_of_ids[] = {
> + { .compatible = "renesas,rcar-rpc-spi", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, rpc_spi_of_ids);

Not needed at all...

[...]

MBR, Sergei

2019-04-02 20:11:49

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver

Hello!

On 04/02/2019 08:48 AM, [email protected] wrote:

>> > +static void rpc_spi_mem_set_prep_op_cfg(struct spi_device *spi,
>> > + const struct spi_mem_op *op,
>> > + u64 *offs, size_t *len)
>> > +{
>> > + struct rpc_spi *rpc = spi_controller_get_devdata(spi->controller);
>> > +
>> > + rpc->cmd = RPC_SMCMR_CMD(op->cmd.opcode);
>> > + rpc->smenr = RPC_SMENR_CDE |
>> > + RPC_SMENR_CDB(ilog2(op->cmd.buswidth));
>> > + rpc->totalxferlen = 1;
>> > + rpc->xfer_dir = SPI_MEM_NO_DATA;
>> > + rpc->xferlen = 0;
>> > + rpc->addr = 0;
>> > +
>> > + if (op->addr.nbytes) {
>> > + rpc->smenr |= RPC_SMENR_ADB(ilog2(op->addr.buswidth));
>> > + if (op->addr.nbytes == 4)
>> > + rpc->smenr |= RPC_SMENR_ADE(0xf);
>> > + else
>> > + rpc->smenr |= RPC_SMENR_ADE(0x7);
>> > +
>> > + if (offs && len)
>> > + rpc->addr = *offs;
>> > + else
>> > + rpc->addr = op->addr.val;
>> > + rpc->totalxferlen += op->addr.nbytes;
>> > + }
>> > +
>> > + if (op->dummy.nbytes) {
>> > + rpc->smenr |= RPC_SMENR_DME;
>> > + rpc->dummy = RPC_SMDMCR_DMCYC(op->dummy.nbytes);
>>
>> So you haven't fixed this bug? I repeat, the driver doesn't work right
>> w/o this fixed!
>
> Do you mean
>
> rpc->dummy = RPC_SMDMCR_DMCYC(op->dummy.nbytes) * 8; ?

Yes. Or some other code that amounts to it.

> What is your flash part number?

Spansion/Cypress S25FS512S. The datasheet says there must be 8 dummy cycles
for the RSFDP command...

> The problem you found is in 1 bit or 4 bits width ?

1-bit, I think.

>>
>> [...]
>> > +static ssize_t rpc_spi_mem_dirmap_read(struct spi_mem_dirmap_desc *desc,
>> > + u64 offs, size_t len, void *buf)
>> > +{
>> > + struct rpc_spi *rpc =
>> > + spi_controller_get_devdata(desc->mem->spi->controller);
>> > + int ret;
>> > +
>> > + if (offs + desc->info.offset + len > U32_MAX)
>> > + return -EINVAL;
>> > +
>> > + if (len > 0x4000000)
>> > + len = 0x4000000;
>>
>> Ugh...
>
> by mtd->size ?

That'd be better, yes.

>> > +
>> > + ret = rpc_spi_set_freq(rpc, desc->mem->spi->max_speed_hz);
>> > + if (ret)
>> > + return ret;
>> > +
>> > + rpc_spi_mem_set_prep_op_cfg(desc->mem->spi,
>> > + &desc->info.op_tmpl, &offs, &len);
>> > +
>> > + regmap_update_bits(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD, 0);
>> > + regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RBURST(32) |
>> > + RPC_DRCR_RBE);
>> > +
>> > + regmap_write(rpc->regmap, RPC_DRCMR, rpc->cmd);
>> > + regmap_write(rpc->regmap, RPC_DREAR, RPC_DREAR_EAC(1));
>>
>> So you're not even trying to support flashes larger than the read dirmap?
>> Now I don't think it's acceptable (and I have rewritten this code internally).
>
> what about the size comes form mtd->size ?

I'm not talking about size here; we should use the full address. I'm attaching
my patch...

>> [...]
>> > +static ssize_t rpc_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
>> > + u64 offs, size_t len, const void *buf)
>> > +{
>> > + struct rpc_spi *rpc =
>> > + spi_controller_get_devdata(desc->mem->spi->controller);
>> > + int ret;
>> > +
>> > + if (offs + desc->info.offset + len > U32_MAX)
>> > + return -EINVAL;
>> > +
>> > + if (len > RPC_WBUF_SIZE)
>> > + len = RPC_WBUF_SIZE;
>>
>> Ugh! Again, I no longer think this is acceptable... Maybe we just need
>> to drop the support of the write buffer...
>>
>
> why ?
> Using write buffer got the much better write performance.

OK, let's keep it then.

[...]

MBR, Sergei


Attachments:
spi-renesas-rpc-allow-reading-from-large-flashes.patch (2.62 kB)

2019-04-04 19:13:55

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver

On Thu, 4 Apr 2019 21:56:19 +0300
Sergei Shtylyov <[email protected]> wrote:

> Hello!
>
> On 04/03/2019 12:20 PM, [email protected] wrote:
>
> >> >> > +static void rpc_spi_mem_set_prep_op_cfg(struct spi_device *spi,
> >> >> > + const struct spi_mem_op *op,
> >> >> > + u64 *offs, size_t *len)
> >> >> > +{
> >> >> > + struct rpc_spi *rpc = spi_controller_get_devdata(spi->controller);
> >> >> > +
> >> >> > + rpc->cmd = RPC_SMCMR_CMD(op->cmd.opcode);
> >> >> > + rpc->smenr = RPC_SMENR_CDE |
> >> >> > + RPC_SMENR_CDB(ilog2(op->cmd.buswidth));
> >> >> > + rpc->totalxferlen = 1;
> >> >> > + rpc->xfer_dir = SPI_MEM_NO_DATA;
> >> >> > + rpc->xferlen = 0;
> >> >> > + rpc->addr = 0;
> >> >> > +
> >> >> > + if (op->addr.nbytes) {
> >> >> > + rpc->smenr |= RPC_SMENR_ADB(ilog2(op->addr.buswidth));
> >> >> > + if (op->addr.nbytes == 4)
> >> >> > + rpc->smenr |= RPC_SMENR_ADE(0xf);
> >> >> > + else
> >> >> > + rpc->smenr |= RPC_SMENR_ADE(0x7);
> >> >> > +
> >> >> > + if (offs && len)
> >> >> > + rpc->addr = *offs;
> >> >> > + else
> >> >> > + rpc->addr = op->addr.val;
> >> >> > + rpc->totalxferlen += op->addr.nbytes;
> >> >> > + }
> >> >> > +
> >> >> > + if (op->dummy.nbytes) {
> >> >> > + rpc->smenr |= RPC_SMENR_DME;
> >> >> > + rpc->dummy = RPC_SMDMCR_DMCYC(op->dummy.nbytes);
> >> >>
> >> >> So you haven't fixed this bug? I repeat, the driver doesn't work right
> >> >> w/o this fixed!
> >> >
> >> > Do you mean
> >> >
> >> > rpc->dummy = RPC_SMDMCR_DMCYC(op->dummy.nbytes) * 8; ?
> >>
> >> Yes. Or some other code that amounts to it.
>
> Oops, I wasn't attentive enough, I was proposing:
>
> rpc->dummy = RPC_SMDMCR_DMCYC(op->dummy.nbytes * 8);
>
> > why not setting "op->dummy.nbytes = 7" from spi-nor.c protocol layer ?
>
> We want 8 dummy clocks, not 8 dummy bytes. And if you'd looked into
> spi_nor_read_sfdp(), you'd have seen that it requests 8 dummy clocks already.
>
> > driver may check device ID or something else to setup op->dummy.nbytes.
>
> The RDSFDP command is not chip specific.
>
> > I think it is not a good idea to *8 in spi host driver.
>
> >> > What is your flash part number?
> >>
> >> Spansion/Cypress S25FS512S. The datasheet says there must be 8 dummy cycles
> >> for the RSFDP command...
> >>
> >> > The problem you found is in 1 bit or 4 bits width ?
> >>
> >> 1-bit, I think.
> >>
> >> >>
> >> >> [...]
> >> >> > +static ssize_t rpc_spi_mem_dirmap_read(struct spi_mem_dirmap_desc *desc,
> >> >> > + u64 offs, size_t len, void *buf)
> >> >> > +{
> >> >> > + struct rpc_spi *rpc =
> >> >> > + spi_controller_get_devdata(desc->mem->spi->controller);
> >> >> > + int ret;
> >> >> > +
> >> >> > + if (offs + desc->info.offset + len > U32_MAX)
> >> >> > + return -EINVAL;
> >> >> > +
> >> >> > + if (len > 0x4000000)
> >> >> > + len = 0x4000000;
> >> >>
> >> >> Ugh...
> >> >
> >> > by mtd->size ?
> >>
> >> That'd be better, yes.
> >>
> >
> > Oops, it seems hard to get mtd->size info. from spi_mem_dirmap,
>
> It's in desc->info.length, no?

It's the lengths of the mapping which not necessarily mtd->size, but in
the SPI NOR case it is :-). Anyway, you should not assume
dirmapdesc->info.length == memory_device->size.

>
> > I would like to keep 0x4000000.
>
> I'm seeing Boris in the CC's... Boris, is it legitimate to limit
> a single dirmap read by the memory "window" size? Or should we try to
> serve any valid transfer length?

If by memory window you're talking about the memory region reserved in
the CPU address space, then no. It should not be limited to this size
if possible. Most HW have a way to configure an offset to apply to the
spi-mem operation, and in that case, the driver should change this
offset on the fly when one tries to access a region that's outside of
the currently configured window.

>
> >> >> > +
> >> >> > + ret = rpc_spi_set_freq(rpc, desc->mem->spi->max_speed_hz);
> >> >> > + if (ret)
> >> >> > + return ret;
> >> >> > +
> >> >> > + rpc_spi_mem_set_prep_op_cfg(desc->mem->spi,
> >> >> > + &desc->info.op_tmpl, &offs, &len);
> >> >> > +
> >> >> > + regmap_update_bits(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD, 0);
> >> >> > + regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RBURST(32) |
> >> >> > + RPC_DRCR_RBE);
> >> >> > +
> >> >> > + regmap_write(rpc->regmap, RPC_DRCMR, rpc->cmd);
> >> >> > + regmap_write(rpc->regmap, RPC_DREAR, RPC_DREAR_EAC(1));
> >> >>
> >> >> So you're not even trying to support flashes larger than the
> >> read dirmap?
> >> >> Now I don't think it's acceptable (and I have rewritten this code
> >> internally).
> >> >
> >> > what about the size comes form mtd->size ?
> >>
> >> I'm not talking about size here; we should use the full address.
> >> I'm attaching
> >> my patch...
> >
> > okay,got it!
> > what about just
> > - regmap_write(rpc->mfd->regmap, RPC_DREAR, RPC_DREAR_EAC(1));
> > + regmap_write(rpc->mfd->regmap, RPC_DREAR,
> > + RPC_DREAR_EAV(offs >> 25) | RPC_DREAR_EAC(1));
> >
> > because only > 64MByte size make RPC_DREAR_EAV() work.
>
> Boris, what's your opinion on this?
> Note that for the write dirmap we have just 256-byte buffer (reusing
> the read cache). Is it legitimate to limit the served length to 256 bytes?

I don't know what the HW is capable of, but drivers should use any try
they have to dynamically move the memory map window (make it point at a
different spi-mem offset on demand). Note that dirmap_read/write() are
allowed to return less than the amount of data requested, in that case
the caller should continue reading at the offset where things stopped.
This avoids having to implement a loop that splits things in several
accesses when the access cannot be done in one step.

2019-04-04 20:25:42

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver

Hello!

On 04/03/2019 12:20 PM, [email protected] wrote:

>> >> > +static void rpc_spi_mem_set_prep_op_cfg(struct spi_device *spi,
>> >> > + const struct spi_mem_op *op,
>> >> > + u64 *offs, size_t *len)
>> >> > +{
>> >> > + struct rpc_spi *rpc = spi_controller_get_devdata(spi->controller);
>> >> > +
>> >> > + rpc->cmd = RPC_SMCMR_CMD(op->cmd.opcode);
>> >> > + rpc->smenr = RPC_SMENR_CDE |
>> >> > + RPC_SMENR_CDB(ilog2(op->cmd.buswidth));
>> >> > + rpc->totalxferlen = 1;
>> >> > + rpc->xfer_dir = SPI_MEM_NO_DATA;
>> >> > + rpc->xferlen = 0;
>> >> > + rpc->addr = 0;
>> >> > +
>> >> > + if (op->addr.nbytes) {
>> >> > + rpc->smenr |= RPC_SMENR_ADB(ilog2(op->addr.buswidth));
>> >> > + if (op->addr.nbytes == 4)
>> >> > + rpc->smenr |= RPC_SMENR_ADE(0xf);
>> >> > + else
>> >> > + rpc->smenr |= RPC_SMENR_ADE(0x7);
>> >> > +
>> >> > + if (offs && len)
>> >> > + rpc->addr = *offs;
>> >> > + else
>> >> > + rpc->addr = op->addr.val;
>> >> > + rpc->totalxferlen += op->addr.nbytes;
>> >> > + }
>> >> > +
>> >> > + if (op->dummy.nbytes) {
>> >> > + rpc->smenr |= RPC_SMENR_DME;
>> >> > + rpc->dummy = RPC_SMDMCR_DMCYC(op->dummy.nbytes);
>> >>
>> >> So you haven't fixed this bug? I repeat, the driver doesn't work right
>> >> w/o this fixed!
>> >
>> > Do you mean
>> >
>> > rpc->dummy = RPC_SMDMCR_DMCYC(op->dummy.nbytes) * 8; ?
>>
>> Yes. Or some other code that amounts to it.

Oops, I wasn't attentive enough, I was proposing:

rpc->dummy = RPC_SMDMCR_DMCYC(op->dummy.nbytes * 8);

> why not setting "op->dummy.nbytes = 7" from spi-nor.c protocol layer ?

We want 8 dummy clocks, not 8 dummy bytes. And if you'd looked into
spi_nor_read_sfdp(), you'd have seen that it requests 8 dummy clocks already.

> driver may check device ID or something else to setup op->dummy.nbytes.

The RDSFDP command is not chip specific.

> I think it is not a good idea to *8 in spi host driver.

>> > What is your flash part number?
>>
>> Spansion/Cypress S25FS512S. The datasheet says there must be 8 dummy cycles
>> for the RSFDP command...
>>
>> > The problem you found is in 1 bit or 4 bits width ?
>>
>> 1-bit, I think.
>>
>> >>
>> >> [...]
>> >> > +static ssize_t rpc_spi_mem_dirmap_read(struct spi_mem_dirmap_desc *desc,
>> >> > + u64 offs, size_t len, void *buf)
>> >> > +{
>> >> > + struct rpc_spi *rpc =
>> >> > + spi_controller_get_devdata(desc->mem->spi->controller);
>> >> > + int ret;
>> >> > +
>> >> > + if (offs + desc->info.offset + len > U32_MAX)
>> >> > + return -EINVAL;
>> >> > +
>> >> > + if (len > 0x4000000)
>> >> > + len = 0x4000000;
>> >>
>> >> Ugh...
>> >
>> > by mtd->size ?
>>
>> That'd be better, yes.
>>
>
> Oops, it seems hard to get mtd->size info. from spi_mem_dirmap,

It's in desc->info.length, no?

> I would like to keep 0x4000000.

I'm seeing Boris in the CC's... Boris, is it legitimate to limit
a single dirmap read by the memory "window" size? Or should we try to
serve any valid transfer length?

>> >> > +
>> >> > + ret = rpc_spi_set_freq(rpc, desc->mem->spi->max_speed_hz);
>> >> > + if (ret)
>> >> > + return ret;
>> >> > +
>> >> > + rpc_spi_mem_set_prep_op_cfg(desc->mem->spi,
>> >> > + &desc->info.op_tmpl, &offs, &len);
>> >> > +
>> >> > + regmap_update_bits(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD, 0);
>> >> > + regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RBURST(32) |
>> >> > + RPC_DRCR_RBE);
>> >> > +
>> >> > + regmap_write(rpc->regmap, RPC_DRCMR, rpc->cmd);
>> >> > + regmap_write(rpc->regmap, RPC_DREAR, RPC_DREAR_EAC(1));
>> >>
>> >> So you're not even trying to support flashes larger than the
>> read dirmap?
>> >> Now I don't think it's acceptable (and I have rewritten this code
>> internally).
>> >
>> > what about the size comes form mtd->size ?
>>
>> I'm not talking about size here; we should use the full address.
>> I'm attaching
>> my patch...
>
> okay,got it!
> what about just
> - regmap_write(rpc->mfd->regmap, RPC_DREAR, RPC_DREAR_EAC(1));
> + regmap_write(rpc->mfd->regmap, RPC_DREAR,
> + RPC_DREAR_EAV(offs >> 25) | RPC_DREAR_EAC(1));
>
> because only > 64MByte size make RPC_DREAR_EAV() work.

Boris, what's your opinion on this?
Note that for the write dirmap we have just 256-byte buffer (reusing
the read cache). Is it legitimate to limit the served length to 256 bytes?

> thanks & best regards,
> Mason

MBR, Sergei

2019-04-06 20:01:25

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver

On 04/04/2019 10:12 PM, Boris Brezillon wrote:

[...]
>>>>>>> +static ssize_t rpc_spi_mem_dirmap_read(struct spi_mem_dirmap_desc *desc,
>>>>>>> + u64 offs, size_t len, void *buf)
>>>>>>> +{
>>>>>>> + struct rpc_spi *rpc =
>>>>>>> + spi_controller_get_devdata(desc->mem->spi->controller);
>>>>>>> + int ret;
>>>>>>> +
>>>>>>> + if (offs + desc->info.offset + len > U32_MAX)
>>>>>>> + return -EINVAL;
>>>>>>> +
>>>>>>> + if (len > 0x4000000)
>>>>>>> + len = 0x4000000;
>>>>>>
>>>>>> Ugh...
>>>>>
>>>>> by mtd->size ?
>>>>
>>>> That'd be better, yes.
>>>>
>>>
>>> Oops, it seems hard to get mtd->size info. from spi_mem_dirmap,
>>
>> It's in desc->info.length, no?
>
> It's the lengths of the mapping which not necessarily mtd->size, but in
> the SPI NOR case it is :-). Anyway, you should not assume
> dirmapdesc->info.length == memory_device->size.
>
>>
>>> I would like to keep 0x4000000.
>>
>> I'm seeing Boris in the CC's... Boris, is it legitimate to limit
>> a single dirmap read by the memory "window" size? Or should we try to
>> serve any valid transfer length?
>
> If by memory window you're talking about the memory region reserved in

Yes, we have 64 MiB window thru which we can "look into" the large MTD chips.

> the CPU address space, then no. It should not be limited to this size
> if possible.

Mhm, so we're expected to loop incrementing the window address register
in order to serve the full xfer request?

> Most HW have a way to configure an offset to apply to the spi-mem operation,
> and in that case, the driver should change this
> offset on the fly when one tries to access a region that's outside of
> the currently configured window.

Well, my question wasn't about that actually -- that seemed quite obvious.

>>>>>>> +
>>>>>>> + ret = rpc_spi_set_freq(rpc, desc->mem->spi->max_speed_hz);
>>>>>>> + if (ret)
>>>>>>> + return ret;
>>>>>>> +
>>>>>>> + rpc_spi_mem_set_prep_op_cfg(desc->mem->spi,
>>>>>>> + &desc->info.op_tmpl, &offs, &len);
>>>>>>> +
>>>>>>> + regmap_update_bits(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD, 0);
>>>>>>> + regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RBURST(32) |
>>>>>>> + RPC_DRCR_RBE);
>>>>>>> +
>>>>>>> + regmap_write(rpc->regmap, RPC_DRCMR, rpc->cmd);
>>>>>>> + regmap_write(rpc->regmap, RPC_DREAR, RPC_DREAR_EAC(1));
>>>>>>
>>>>>> So you're not even trying to support flashes larger than the
>>>> read dirmap?
>>>>>> Now I don't think it's acceptable (and I have rewritten this code
>>>> internally).
>>>>>
>>>>> what about the size comes form mtd->size ?
>>>>
>>>> I'm not talking about size here; we should use the full address.
>>>> I'm attaching
>>>> my patch...
>>>
>>> okay,got it!
>>> what about just
>>> - regmap_write(rpc->mfd->regmap, RPC_DREAR, RPC_DREAR_EAC(1));
>>> + regmap_write(rpc->mfd->regmap, RPC_DREAR,
>>> + RPC_DREAR_EAV(offs >> 25) | RPC_DREAR_EAC(1));
>>>
>>> because only > 64MByte size make RPC_DREAR_EAV() work.
>>
>> Boris, what's your opinion on this?
>> Note that for the write dirmap we have just 256-byte buffer (reusing
>> the read cache). Is it legitimate to limit the served length to 256 bytes?

> I don't know what the HW is capable of,

As I said, there's 64 MiB read window, and for the writes we can re-use the
read cache to write (exactly) 256 bytes at a time...

> but drivers should use any try
> they have to dynamically move the memory map window (make it point at a
> different spi-mem offset on demand). Note that dirmap_read/write() are
> allowed to return less than the amount of data requested, in that case
> the caller should continue reading at the offset where things stopped.
> This avoids having to implement a loop that splits things in several
> accesses when the access cannot be done in one step.

Ah, this somewhat contradicts what you said earlier but seems clear now.
I'll go remove the loops. :-)

MBR, Sergei

2019-04-09 11:23:39

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver

On 04/03/2019 12:20 PM, [email protected] wrote:

[...]
>> >> > + ret = rpc_spi_set_freq(rpc, desc->mem->spi->max_speed_hz);
>> >> > + if (ret)
>> >> > + return ret;
>> >> > +
>> >> > + rpc_spi_mem_set_prep_op_cfg(desc->mem->spi,
>> >> > + &desc->info.op_tmpl, &offs, &len);
>> >> > +
>> >> > + regmap_update_bits(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD, 0);
>> >> > + regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RBURST(32) |
>> >> > + RPC_DRCR_RBE);
>> >> > +
>> >> > + regmap_write(rpc->regmap, RPC_DRCMR, rpc->cmd);
>> >> > + regmap_write(rpc->regmap, RPC_DREAR, RPC_DREAR_EAC(1));
>> >>
>> >> So you're not even trying to support flashes larger than the
>> read dirmap?
>> >> Now I don't think it's acceptable (and I have rewritten this code
>> internally).
>> >
>> > what about the size comes form mtd->size ?
>>
>> I'm not talking about size here; we should use the full address.
>> I'm attaching
>> my patch...
>
> okay,got it!
> what about just
> - regmap_write(rpc->mfd->regmap, RPC_DREAR, RPC_DREAR_EAC(1));
> + regmap_write(rpc->mfd->regmap, RPC_DREAR,
> + RPC_DREAR_EAV(offs >> 25) | RPC_DREAR_EAC(1));
>
> because only > 64MByte size make RPC_DREAR_EAV() work.

Seems OK now, after Boris' reply.

> thanks & best regards,
> Mason

[...]

MBR, Sergei

2019-04-09 20:08:13

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver

Hello!

On 03/29/2019 11:20 AM, Mason Yang wrote:

> Add a driver for Renesas R-Car Gen3 RPC-IF SPI controller.
>
> Signed-off-by: Mason Yang <[email protected]>
> Signed-off-by: Sergei Shtylyov <[email protected]>
[...]

> diff --git a/drivers/spi/spi-renesas-rpc.c b/drivers/spi/spi-renesas-rpc.c
> new file mode 100644
> index 0000000..037f273
> --- /dev/null
> +++ b/drivers/spi/spi-renesas-rpc.c
[...]
[...]
> +static ssize_t rpc_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
> + u64 offs, size_t len, const void *buf)
> +{
> + struct rpc_spi *rpc =
> + spi_controller_get_devdata(desc->mem->spi->controller);
> + int ret;
> +
> + if (offs + desc->info.offset + len > U32_MAX)
> + return -EINVAL;
> +
> + if (len > RPC_WBUF_SIZE)
> + len = RPC_WBUF_SIZE;
> +
> + ret = rpc_spi_set_freq(rpc, desc->mem->spi->max_speed_hz);
> + if (ret)
> + return ret;
> +
> + rpc_spi_mem_set_prep_op_cfg(desc->mem->spi,
> + &desc->info.op_tmpl, &offs, &len);
> +
> + regmap_update_bits(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD, RPC_CMNCR_MD);
> +
> + regmap_write(rpc->regmap, RPC_SMDRENR, 0);
> +
> + regmap_update_bits(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_STRTIM(7) |
> + RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF,
> + RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF);
> +
> + memcpy_toio(rpc->wbuf, buf, len);

Wait, doesn't the manual say that the whole 256-byte buffer should be
filled? I think that short chunks have to be written w/o WBUF (done, in fact,
by the HF driver).

> +
> + regmap_write(rpc->regmap, RPC_SMCMR, rpc->cmd);
> + regmap_write(rpc->regmap, RPC_SMADR, offs);
> + regmap_write(rpc->regmap, RPC_SMENR, rpc->smenr);
> + regmap_write(rpc->regmap, RPC_SMCR, rpc->smcr | RPC_SMCR_SPIE);
> + ret = wait_msg_xfer_end(rpc);
> + if (ret)
> + goto err_out;
> +
> + regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RCF);
> +
> + regmap_update_bits(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_STRTIM(7) |
> + RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF,
> + RPC_PHYCNT_STRTIM(6));
> +
> + return len;
> +
> +err_out:
> + return reset_control_reset(rpc->rstc);
> +}
[...]

MBR, Sergei

2019-04-10 06:42:08

by Marek Vasut

[permalink] [raw]
Subject: Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver

On 4/10/19 3:33 AM, [email protected] wrote:
> Hi Sergei,
>
>> > +static ssize_t rpc_spi_mem_dirmap_write(struct spi_mem_dirmap_desc
> *desc,
>> > + ? ? ? ? ? ? ? u64 offs, size_t len, const void *buf)
>> > +{
>> > + ? struct rpc_spi *rpc =
>> > + ? ? ? ? spi_controller_get_devdata(desc->mem->spi->controller);
>> > + ? int ret;
>> > +
>> > + ? if (offs + desc->info.offset + len > U32_MAX)
>> > + ? ? ?return -EINVAL;
>> > +
>> > + ? if (len > RPC_WBUF_SIZE)
>> > + ? ? ?len = RPC_WBUF_SIZE;
>> > +
>> > + ? ret = rpc_spi_set_freq(rpc, desc->mem->spi->max_speed_hz);
>> > + ? if (ret)
>> > + ? ? ?return ret;
>> > +
>> > + ? rpc_spi_mem_set_prep_op_cfg(desc->mem->spi,
>> > + ? ? ? ? ? ? ? ?&desc->info.op_tmpl, &offs, &len);
>> > +
>> > + ? regmap_update_bits(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD,
> RPC_CMNCR_MD);
>> > +
>> > + ? regmap_write(rpc->regmap, RPC_SMDRENR, 0);
>> > +
>> > + ? regmap_update_bits(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_STRTIM(7) |
>> > + ? ? ? ? ? ?RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF,
>> > + ? ? ? ? ? ?RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF);
>> > +
>> > + ? memcpy_toio(rpc->wbuf, buf, len);
>>
>> ? ?Wait, doesn't the manual say that the whole 256-byte buffer should be
>> filled? ?I think that short chunks have to be written w/o WBUF (done,
> in fact,
>> by the HF driver).
>>
>
> From spi-nor.c layer always transfer 256 bytes data with page program
> command.

Does that apply even for flashes with not-256-byte pages ?

--
Best regards,
Marek Vasut

2019-04-10 09:15:09

by Marek Vasut

[permalink] [raw]
Subject: Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver

On 4/10/19 10:01 AM, [email protected] wrote:
> Hi,
> ?
>> Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller
> driver
>>
>> On 4/10/19 3:33 AM, [email protected] wrote:
>> > Hi Sergei,
>> >
>> >> > +static ssize_t rpc_spi_mem_dirmap_write(struct spi_mem_dirmap_desc
>> > *desc,
>> >> > + ? ? ? ? ? ? ? u64 offs, size_t len, const void *buf)
>> >> > +{
>> >> > + ? struct rpc_spi *rpc =
>> >> > + ? ? ? ? spi_controller_get_devdata(desc->mem->spi->controller);
>> >> > + ? int ret;
>> >> > +
>> >> > + ? if (offs + desc->info.offset + len > U32_MAX)
>> >> > + ? ? ?return -EINVAL;
>> >> > +
>> >> > + ? if (len > RPC_WBUF_SIZE)
>> >> > + ? ? ?len = RPC_WBUF_SIZE;
>> >> > +
>> >> > + ? ret = rpc_spi_set_freq(rpc, desc->mem->spi->max_speed_hz);
>> >> > + ? if (ret)
>> >> > + ? ? ?return ret;
>> >> > +
>> >> > + ? rpc_spi_mem_set_prep_op_cfg(desc->mem->spi,
>> >> > + ? ? ? ? ? ? ? ?&desc->info.op_tmpl, &offs, &len);
>> >> > +
>> >> > + ? regmap_update_bits(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD,
>> > RPC_CMNCR_MD);
>> >> > +
>> >> > + ? regmap_write(rpc->regmap, RPC_SMDRENR, 0);
>> >> > +
>> >> > + ? regmap_update_bits(rpc->regmap, RPC_PHYCNT,
> RPC_PHYCNT_STRTIM(7) |
>> >> > + ? ? ? ? ? ?RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF,
>> >> > + ? ? ? ? ? ?RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF);
>> >> > +
>> >> > + ? memcpy_toio(rpc->wbuf, buf, len);
>> >>
>> >> ? ?Wait, doesn't the manual say that the whole 256-byte buffer
> should be
>> >> filled?
>
> it could be less than 256 bytes, i.e., 128 bytes to rpc->wbuf !
>
> ?I think that short chunks have to be written w/o WBUF (done,
>> > in fact,
>> >> by the HF driver).
>> >>
>> >
>> > From spi-nor.c layer always transfer 256 bytes data with page program
>> > command.
>>
>> Does that apply even for flashes with not-256-byte pages ?
>>
>
> I think it needs to patch in case of nor->page_size = 512 bytes.

There are also flashes with page size < 256 bytes :)

--
Best regards,
Marek Vasut

2019-04-10 09:17:01

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver

On Wed, 10 Apr 2019 16:01:02 +0800
[email protected] wrote:

> Hi,
>
> > Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller
> driver
> >
> > On 4/10/19 3:33 AM, [email protected] wrote:
> > > Hi Sergei,
> > >
> > >> > +static ssize_t rpc_spi_mem_dirmap_write(struct spi_mem_dirmap_desc
> > > *desc,
> > >> > + u64 offs, size_t len, const void *buf)
> > >> > +{
> > >> > + struct rpc_spi *rpc =
> > >> > + spi_controller_get_devdata(desc->mem->spi->controller);
> > >> > + int ret;
> > >> > +
> > >> > + if (offs + desc->info.offset + len > U32_MAX)
> > >> > + return -EINVAL;
> > >> > +
> > >> > + if (len > RPC_WBUF_SIZE)
> > >> > + len = RPC_WBUF_SIZE;
> > >> > +
> > >> > + ret = rpc_spi_set_freq(rpc, desc->mem->spi->max_speed_hz);
> > >> > + if (ret)
> > >> > + return ret;
> > >> > +
> > >> > + rpc_spi_mem_set_prep_op_cfg(desc->mem->spi,
> > >> > + &desc->info.op_tmpl, &offs, &len);
> > >> > +
> > >> > + regmap_update_bits(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD,
> > > RPC_CMNCR_MD);
> > >> > +
> > >> > + regmap_write(rpc->regmap, RPC_SMDRENR, 0);
> > >> > +
> > >> > + regmap_update_bits(rpc->regmap, RPC_PHYCNT,
> RPC_PHYCNT_STRTIM(7) |
> > >> > + RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF,
> > >> > + RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF);
> > >> > +
> > >> > + memcpy_toio(rpc->wbuf, buf, len);
> > >>
> > >> Wait, doesn't the manual say that the whole 256-byte buffer should
> be
> > >> filled?
>
> it could be less than 256 bytes, i.e., 128 bytes to rpc->wbuf !
>
> I think that short chunks have to be written w/o WBUF (done,
> > > in fact,
> > >> by the HF driver).
> > >>
> > >
> > > From spi-nor.c layer always transfer 256 bytes data with page program
> > > command.
> >
> > Does that apply even for flashes with not-256-byte pages ?
> >
>
> I think it needs to patch in case of nor->page_size = 512 bytes.

I think the main problem here is that you assume the memory is a
NOR :-). Just do what the spi-mem user asks: if he asks you to write
X bytes, then write no more than X bytes. Use whatever trick you have
to make sure this is always true, and if this requires using a slow
path for non-aligned accesses, then do it, because it's better to have
a slow+working memory than a fast+non-working one :P.

2019-04-10 19:09:00

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver

Hello!

On 04/10/2019 11:01 AM, [email protected] wrote:

>> >> > +static ssize_t rpc_spi_mem_dirmap_write(struct spi_mem_dirmap_desc
>> > *desc,
>> >> > + u64 offs, size_t len, const void *buf)
>> >> > +{
>> >> > + struct rpc_spi *rpc =
>> >> > + spi_controller_get_devdata(desc->mem->spi->controller);
>> >> > + int ret;
>> >> > +
>> >> > + if (offs + desc->info.offset + len > U32_MAX)
>> >> > + return -EINVAL;
>> >> > +
>> >> > + if (len > RPC_WBUF_SIZE)
>> >> > + len = RPC_WBUF_SIZE;
>> >> > +
>> >> > + ret = rpc_spi_set_freq(rpc, desc->mem->spi->max_speed_hz);
>> >> > + if (ret)
>> >> > + return ret;
>> >> > +
>> >> > + rpc_spi_mem_set_prep_op_cfg(desc->mem->spi,
>> >> > + &desc->info.op_tmpl, &offs, &len);
>> >> > +
>> >> > + regmap_update_bits(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD,
>> > RPC_CMNCR_MD);
>> >> > +
>> >> > + regmap_write(rpc->regmap, RPC_SMDRENR, 0);
>> >> > +
>> >> > + regmap_update_bits(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_STRTIM(7) |
>> >> > + RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF,
>> >> > + RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF);
>> >> > +
>> >> > + memcpy_toio(rpc->wbuf, buf, len);
>> >>
>> >> Wait, doesn't the manual say that the whole 256-byte buffer should be
>> >> filled?
>
> it could be less than 256 bytes, i.e., 128 bytes to rpc->wbuf !

The gen3 manual 1.50 contradicts:

Note: This area should be accessed sequentially from the start address and transfer size
to the device is 256-byte unit. All Write Buffer area should be filled with transfer data
in every transfer. When accessing non-sequentially or at random, the operation is not
guaranteed. After Write Buffer Operation, this cache area must be cleared by DRCR.RCF bit.

> I think that short chunks have to be written w/o WBUF (done,
>> > in fact,
>> >> by the HF driver).
>> >>
>> >
>> > From spi-nor.c layer always transfer 256 bytes data with page program
>> > command.
>>
>> Does that apply even for flashes with not-256-byte pages ?
>>
>
> I think it needs to patch in case of nor->page_size = 512 bytes.

What needs to patch what? :-)

> thanks & best regards,
> Mason

MBR, Sergei

2019-04-10 19:48:41

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver

On 04/09/2019 02:20 PM, Sergei Shtylyov wrote:

> [...]
>>>>>> + ret = rpc_spi_set_freq(rpc, desc->mem->spi->max_speed_hz);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + rpc_spi_mem_set_prep_op_cfg(desc->mem->spi,
>>>>>> + &desc->info.op_tmpl, &offs, &len);
>>>>>> +
>>>>>> + regmap_update_bits(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD, 0);
>>>>>> + regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RBURST(32) |
>>>>>> + RPC_DRCR_RBE);
>>>>>> +
>>>>>> + regmap_write(rpc->regmap, RPC_DRCMR, rpc->cmd);
>>>>>> + regmap_write(rpc->regmap, RPC_DREAR, RPC_DREAR_EAC(1));
>>>>>
>>>>> So you're not even trying to support flashes larger than the
>>> read dirmap?
>>>>> Now I don't think it's acceptable (and I have rewritten this code
>>> internally).
>>>>
>>>> what about the size comes form mtd->size ?
>>>
>>> I'm not talking about size here; we should use the full address.
>>> I'm attaching
>>> my patch...
>>
>> okay,got it!
>> what about just
>> - regmap_write(rpc->mfd->regmap, RPC_DREAR, RPC_DREAR_EAC(1));
>> + regmap_write(rpc->mfd->regmap, RPC_DREAR,
>> + RPC_DREAR_EAV(offs >> 25) | RPC_DREAR_EAC(1));
>>
>> because only > 64MByte size make RPC_DREAR_EAV() work.
>
> Seems OK now, after Boris' reply.

Well, actually not. The 'len' check in the beginning is oversimplified.
Attached is the patch fixing up the dirmap read path (and making it handle
flashes > 64 MiB as well)...

>> thanks & best regards,
>> Mason
>
> [...]

MBR, Sergei


Attachments:
spi-renesas-rpc-fix-dirmap-read.patch (2.33 kB)

2019-04-13 16:39:31

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver

Hello!

On 03/29/2019 11:20 AM, Mason Yang wrote:

> Add a driver for Renesas R-Car Gen3 RPC-IF SPI controller.
>
> Signed-off-by: Mason Yang <[email protected]>
> Signed-off-by: Sergei Shtylyov <[email protected]>
[...]
> diff --git a/drivers/spi/spi-renesas-rpc.c b/drivers/spi/spi-renesas-rpc.c
> new file mode 100644
> index 0000000..037f273
> --- /dev/null
> +++ b/drivers/spi/spi-renesas-rpc.c
[...]
> +static int rpc_spi_probe(struct platform_device *pdev)
> +{
> + struct spi_controller *ctlr;
> + struct rpc_mfd *rpc_mfd = dev_get_drvdata(pdev->dev.parent);
> + struct rpc_spi *rpc;
> + int ret;
> +
> + ctlr = spi_alloc_master(&pdev->dev, sizeof(*rpc));
> + if (!ctlr)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, ctlr);
> +
> + rpc = spi_controller_get_devdata(ctlr);
> +
> + ctlr->dev.of_node = pdev->dev.of_node;
[...]
> +
> + pm_runtime_enable(&pdev->dev);
> + ctlr->auto_runtime_pm = true;

I think this line no longer works as expected with the new probing scheme.
Have you tested reading? v8 patch still works while v9 patches hang on doing:

$ cat /dev/mtd<n>...

[...]

2019-04-13 16:40:50

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver

On 04/13/2019 07:38 PM, Sergei Shtylyov wrote:

>> Add a driver for Renesas R-Car Gen3 RPC-IF SPI controller.
>>
>> Signed-off-by: Mason Yang <[email protected]>
>> Signed-off-by: Sergei Shtylyov <[email protected]>
> [...]
>> diff --git a/drivers/spi/spi-renesas-rpc.c b/drivers/spi/spi-renesas-rpc.c
>> new file mode 100644
>> index 0000000..037f273
>> --- /dev/null
>> +++ b/drivers/spi/spi-renesas-rpc.c
> [...]
>> +static int rpc_spi_probe(struct platform_device *pdev)
>> +{
>> + struct spi_controller *ctlr;
>> + struct rpc_mfd *rpc_mfd = dev_get_drvdata(pdev->dev.parent);
>> + struct rpc_spi *rpc;
>> + int ret;
>> +
>> + ctlr = spi_alloc_master(&pdev->dev, sizeof(*rpc));
>> + if (!ctlr)
>> + return -ENOMEM;
>> +
>> + platform_set_drvdata(pdev, ctlr);
>> +
>> + rpc = spi_controller_get_devdata(ctlr);
>> +
>> + ctlr->dev.of_node = pdev->dev.of_node;
> [...]
>> +
>> + pm_runtime_enable(&pdev->dev);
>> + ctlr->auto_runtime_pm = true;
>
> I think this line no longer works as expected with the new probing scheme.
> Have you tested reading? v8 patch still works while v9 patches hang on doing:
>
> $ cat /dev/mtd<n>...

Sorry, 'od -x', not 'cat'.

WBR, Sergei

2019-04-17 18:45:19

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver

On 04/15/2019 05:36 AM, [email protected] wrote:

>> >> Add a driver for Renesas R-Car Gen3 RPC-IF SPI controller.
>> >>
>> >> Signed-off-by: Mason Yang <[email protected]>
>> >> Signed-off-by: Sergei Shtylyov <[email protected]>
>> > [...]
>> >> diff --git a/drivers/spi/spi-renesas-rpc.c b/drivers/spi/spi-renesas-rpc.c
>> >> new file mode 100644
>> >> index 0000000..037f273
>> >> --- /dev/null
>> >> +++ b/drivers/spi/spi-renesas-rpc.c
>> > [...]
>> >> +static int rpc_spi_probe(struct platform_device *pdev)
>> >> +{
>> >> + struct spi_controller *ctlr;
>> >> + struct rpc_mfd *rpc_mfd = dev_get_drvdata(pdev->dev.parent);
>> >> + struct rpc_spi *rpc;
>> >> + int ret;
>> >> +
>> >> + ctlr = spi_alloc_master(&pdev->dev, sizeof(*rpc));
>> >> + if (!ctlr)
>> >> + return -ENOMEM;
>> >> +
>> >> + platform_set_drvdata(pdev, ctlr);
>> >> +
>> >> + rpc = spi_controller_get_devdata(ctlr);
>> >> +
>> >> + ctlr->dev.of_node = pdev->dev.of_node;
>> > [...]
>> >> +
>> >> + pm_runtime_enable(&pdev->dev);
>> >> + ctlr->auto_runtime_pm = true;
>> >
>> > I think this line no longer works as expected with the new
>> probing scheme.

That's because we added another (SPI) device under our MFD.

>> > Have you tested reading? v8 patch still works while v9 patches
>> > hang on doing:
>> >
>> > $ cat /dev/mtd<n>...
>>
>> Sorry, 'od -x', not 'cat'.
>
> root@draak:/# cat /proc/mtd
> dev: size erasesize name
> mtd0: 00040000 00001000 "Bank 1 - Boot parameter"
> mtd1: 00140000 00001000 "Bank 1 - Loader-BL2"
> mtd2: 00040000 00001000 "Bank 1 - Certification"
> mtd3: 00080000 00001000 "Bank 1 - ARM Trusted FW"
> mtd4: 00400000 00001000 "Bank 1 - Reserved-1"
> mtd5: 00300000 00001000 "Bank 1 - U-Boot"
> mtd6: 00200000 00001000 "Bank 1 - Reserved-2"
> mtd7: 00480000 00001000 "Bank 1 - Splash"
> mtd8: 00040000 00001000 "Bank 1 - Device Tree"
> root@draak:/# od -x /dev/mtd1
> 0000000 0000 d280 0001 d280 0002 d280 0003 d280
> 0000020 0004 d280 0005 d280 0006 d280 0007 d280
> 0000040 0008 d280 0009 d280 000a d280 000b d280
> 0000060 000c d280 000d d280 000e d280 000f d280
> 0000100 0010 d280 0011 d280 0012 d280 0013 d280
> 0000120 0014 d280 0015 d280 0016 d280 0017 d280
> 0000140 0018 d280 0019 d280 001a d280 001b d280
> 0000160 001c d280 001d d280 001e d280 1000 d53e
> 0000200 f800 9266 1000 d51e 3fdf d503 3ba0 1005

Still hangs for me. After I patches spi-mem.c and the driver to
call RPM for the MFD, it started working again. Perhaps, that clock
is still enabled on your target. What does the following print (for
the RPC clocks)?

$ mount none -t debugfs /sys/kernel/debug/
$ cat /sys/kernel/debug/clk/clk_summary

> fyi~
>
> best regards,
> Mason

MBR, Sergei

2019-04-18 19:44:34

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver

On 04/18/2019 05:51 AM, [email protected] wrote:

>> >> >> Add a driver for Renesas R-Car Gen3 RPC-IF SPI controller.
>> >> >>
>> >> >> Signed-off-by: Mason Yang <[email protected]>
>> >> >> Signed-off-by: Sergei Shtylyov <[email protected]>
>> >> > [...]
>> >> >> diff --git a/drivers/spi/spi-renesas-rpc.c b/drivers/spi/spi-
>> renesas-rpc.c
>> >> >> new file mode 100644
>> >> >> index 0000000..037f273
>> >> >> --- /dev/null
>> >> >> +++ b/drivers/spi/spi-renesas-rpc.c
>> >> > [...]
>> >> >> +static int rpc_spi_probe(struct platform_device *pdev)
>> >> >> +{
>> >> >> + struct spi_controller *ctlr;
>> >> >> + struct rpc_mfd *rpc_mfd = dev_get_drvdata(pdev->dev.parent);
>> >> >> + struct rpc_spi *rpc;
>> >> >> + int ret;
>> >> >> +
>> >> >> + ctlr = spi_alloc_master(&pdev->dev, sizeof(*rpc));
>> >> >> + if (!ctlr)
>> >> >> + return -ENOMEM;
>> >> >> +
>> >> >> + platform_set_drvdata(pdev, ctlr);
>> >> >> +
>> >> >> + rpc = spi_controller_get_devdata(ctlr);
>> >> >> +
>> >> >> + ctlr->dev.of_node = pdev->dev.of_node;
>> >> > [...]
>> >> >> +
>> >> >> + pm_runtime_enable(&pdev->dev);
>> >> >> + ctlr->auto_runtime_pm = true;
>> >> >
>> >> > I think this line no longer works as expected with the new
>> >> probing scheme.
>>
>> That's because we added another (SPI) device under our MFD.
>
> Do you mean just to remove one line
> ctlr->auto_runtime_pm = true;
> ?

No, you should explicitly call RPM for the MFD (not the SPI device).

>> >> > Have you tested reading? v8 patch still works while v9 patches
>> >> > hang on doing:
>> >> >
>> >> > $ cat /dev/mtd<n>...
>> >>
>> >> Sorry, 'od -x', not 'cat'.
>> >
>> > root@draak:/# cat /proc/mtd
>> > dev: size erasesize name
>> > mtd0: 00040000 00001000 "Bank 1 - Boot parameter"
>> > mtd1: 00140000 00001000 "Bank 1 - Loader-BL2"
>> > mtd2: 00040000 00001000 "Bank 1 - Certification"
>> > mtd3: 00080000 00001000 "Bank 1 - ARM Trusted FW"
>> > mtd4: 00400000 00001000 "Bank 1 - Reserved-1"
>> > mtd5: 00300000 00001000 "Bank 1 - U-Boot"
>> > mtd6: 00200000 00001000 "Bank 1 - Reserved-2"
>> > mtd7: 00480000 00001000 "Bank 1 - Splash"
>> > mtd8: 00040000 00001000 "Bank 1 - Device Tree"
>> > root@draak:/# od -x /dev/mtd1
>> > 0000000 0000 d280 0001 d280 0002 d280 0003 d280
>> > 0000020 0004 d280 0005 d280 0006 d280 0007 d280
>> > 0000040 0008 d280 0009 d280 000a d280 000b d280
>> > 0000060 000c d280 000d d280 000e d280 000f d280
>> > 0000100 0010 d280 0011 d280 0012 d280 0013 d280
>> > 0000120 0014 d280 0015 d280 0016 d280 0017 d280
>> > 0000140 0018 d280 0019 d280 001a d280 001b d280
>> > 0000160 001c d280 001d d280 001e d280 1000 d53e
>> > 0000200 f800 9266 1000 d51e 3fdf d503 3ba0 1005
>>
>> Still hangs for me. After I patches spi-mem.c and the driver to
>> call RPM for the MFD, it started working again. Perhaps, that clock
>> is still enabled on your target. What does the following print (for
>> the RPC clocks)?
>>
>> $ mount none -t debugfs /sys/kernel/debug/
>> $ cat /sys/kernel/debug/clk/clk_summary
>>
>
> root@draak:/# cat /sys/kernel/debug/clk/clk_summary
> enable prepare protect duty
> clock count count count rate accuracy phase cycle
> ---------------------------------------------------------------------------------------------
> audio_clkout1 0 0 0 11289600 0 0 50000
> x19_clk 0 0 0 24576000 0 0 50000
> dclkin-0 0 0 0 0 0 0 50000
> scif 1 1 0 0 0 0 50000
> audio_clkb 0 0 0 22579200 0 0 50000
> msiof-ref-clock 0 0 0 66666666 0 0 50000
> extal 2 3 0 48000000 0 0 50000
> r 0 2 0 31250 0 0 50000
> rpc-if 0 1 0 31250 0 0 50000

This looks wrong, the RPC-IF module clock should have RPC or RPCD2 (where ae they?)
as a source, not RLCK...

> rwdt 0 1 0 31250 0 0 50000
> cmt0 0 0 0 31250 0 0 50000
> cmt1 0 0 0 31250 0 0 50000
> cmt2 0 0 0 31250 0 0 50000
> cmt3 0 0 0 31250 0 0 50000
> osc 0 0 0 125000 0 0 50000
>
>
> thanks & best regards,
> Mason

MBR, Sergei

2019-04-22 16:49:44

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver

On 04/19/2019 12:31 PM, [email protected] wrote:

>> > Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver
>> >
>> > On 04/18/2019 05:51 AM, [email protected] wrote:
>> >
>> > >> >> >> Add a driver for Renesas R-Car Gen3 RPC-IF SPI controller.
>> > >> >> >>
>> > >> >> >> Signed-off-by: Mason Yang <[email protected]>
>> > >> >> >> Signed-off-by: Sergei Shtylyov <[email protected]>
>> > >> >> > [...]
>> > >> >> >> diff --git a/drivers/spi/spi-renesas-rpc.c b/drivers/spi/spi-
>> > >> renesas-rpc.c
>> > >> >> >> new file mode 100644
>> > >> >> >> index 0000000..037f273
>> > >> >> >> --- /dev/null
>> > >> >> >> +++ b/drivers/spi/spi-renesas-rpc.c
>> > >> >> > [...]
>> > >> >> >> +static int rpc_spi_probe(struct platform_device *pdev)
>> > >> >> >> +{
>> > >> >> >> + struct spi_controller *ctlr;
>> > >> >> >> + struct rpc_mfd *rpc_mfd = dev_get_drvdata(pdev->dev.parent);
>> > >> >> >> + struct rpc_spi *rpc;
>> > >> >> >> + int ret;
>> > >> >> >> +
>> > >> >> >> + ctlr = spi_alloc_master(&pdev->dev, sizeof(*rpc));
>> > >> >> >> + if (!ctlr)
>> > >> >> >> + return -ENOMEM;
>> > >> >> >> +
>> > >> >> >> + platform_set_drvdata(pdev, ctlr);
>> > >> >> >> +
>> > >> >> >> + rpc = spi_controller_get_devdata(ctlr);
>> > >> >> >> +
>> > >> >> >> + ctlr->dev.of_node = pdev->dev.of_node;
>> > >> >> > [...]
>> > >> >> >> +
>> > >> >> >> + pm_runtime_enable(&pdev->dev);
>> > >> >> >> + ctlr->auto_runtime_pm = true;
>> > >> >> >
>> > >> >> > I think this line no longer works as expected with the new
>> > >> >> probing scheme.
>> > >>
>> > >> That's because we added another (SPI) device under our MFD.
>> > >
>> > > Do you mean just to remove one line
>> > > ctlr->auto_runtime_pm = true;
>> > > ?
>
>> how did you test it ?
>> what is your testing flow ?

I think I've patched drivers/spi/spi-mem.c to get spi_mem_access_{start|end}()
to be called with ctlr->dev.parent->parent as an arg...

>> > No, you should explicitly call RPM for the MFD (not the SPI device).
>>
>> okay, patch RPM to RPC MFD and will remove SPI RPM enable part.
>
> wait! I test it ok.
>
> Don't need to patch RPM to RPC MFD, just keep RPM in SPI.
>
> root@draak:/sys/power# echo mem > state
> [ 135.848711] PM: suspend entry (s2idle)
> [ 135.852777] PM: Syncing filesystems ... done.
> [ 135.869468] Freezing user space processes ... (elapsed 0.002 seconds) done.
> [ 135.879962] OOM killer disabled.
> [ 135.883257] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
> [ 135.893474] Suspending console(s) (use no_console_suspend to debug)
> [ 135.910946] [Mason] rpc_spi_suspend
> [ 135.986022] PM: suspend devices took 0.084 seconds
> [ 135.986047] PM: suspend debug: Waiting for 5 second(s).
> [ 141.142024] [Mason] rpc_spi_resume
> [ 141.220859] PM: resume devices took 0.116 seconds
> [ 141.243179] OOM killer enabled.
> [ 141.246417] Restarting tasks ... done.
> [ 141.266579] PM: suspend exit
> root@draak:/sys/power#

That's not *runtime* PM, it's the *plain* PM you're testing here...

> thanks & best regards,
> Mason

MBR, Sergei

2019-04-23 07:30:22

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver

Hi Mason,

On Fri, Apr 19, 2019 at 7:39 AM <[email protected]> wrote:
> > Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver
> > On 04/18/2019 05:51 AM, [email protected] wrote:
> > >> Still hangs for me. After I patches spi-mem.c and the driver to
> > >> call RPM for the MFD, it started working again. Perhaps, that clock
> > >> is still enabled on your target. What does the following print (for
> > >> the RPC clocks)?
> > >>
> > >> $ mount none -t debugfs /sys/kernel/debug/
> > >> $ cat /sys/kernel/debug/clk/clk_summary
> > >>
> > >
> > > root@draak:/# cat /sys/kernel/debug/clk/clk_summary
> > > enable prepare protect duty
> > > clock count count count
> > rate accuracy phase cycle
> > >
> > ---------------------------------------------------------------------------------------------
> > > audio_clkout1 0 0 0 11289600 0 0 50000
> > > x19_clk 0 0 0 24576000 0 0 50000
> > > dclkin-0 0 0 0 0 0 0 50000
> > > scif 1 1 0 0 0 0 50000
> > > audio_clkb 0 0 0 22579200 0 0 50000
> > > msiof-ref-clock 0 0 0 66666666 0 0 50000
> > > extal 2 3 0 48000000 0 0 50000
> > > r 0 2 0 31250 0 0 50000
> > > rpc-if 0 1 0 31250 0 0 50000
> >
> > This looks wrong, the RPC-IF module clock should have RPC or
> > RPCD2 (where ae they?)
> > as a source, not RLCK...
>
> I check the Ch8 CPG of R-Car datasheet, figure 8.1f(R-Car D3),
> the RPC/RPCD2 is derived from PLL1 -> PLL1CK
>
> I didn't patch it and it should be exposed afrer the .sdsrc
>
> .main 2 2 48000000 0 0
> .pll3 0 0 928000000 0 0
> .pll1 4 4 1600000000 0 0
> lv1 0 0 1600000000 0 0
> lv0 1 1 1600000000 0 0
> lvds 1 1 1600000000 0 0
> cl 0 0 33333333 0 0
> zx 0 0 533333333 0 0
> zt 0 0 400000000 0 0
> ztr 0 0 266666666 0 0
> .sdsrc 1 1 800000000 0 0
> sd0 1 1 200000000 0 0
> emmc0
> ---> here !

Upstream drivers/clk/renesas/r8a77995-cpg-mssr.c does not have the "rpc-if"
clock, and we have not yet seen the patch to add it that you must be using
to make your driver work. If the clock is defined wrongly, that might
explain why your driver is working for you, but not for Sergei.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2019-05-14 19:08:18

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver

Hello!

On 04/19/2019 08:38 AM, [email protected] wrote:

>> >> >> >> Add a driver for Renesas R-Car Gen3 RPC-IF SPI controller.
>> >> >> >>
>> >> >> >> Signed-off-by: Mason Yang <[email protected]>
>> >> >> >> Signed-off-by: Sergei Shtylyov <[email protected]>
>> >> >> > [...]
>> >> >> >> diff --git a/drivers/spi/spi-renesas-rpc.c b/drivers/spi/spi-
>> >> renesas-rpc.c
>> >> >> >> new file mode 100644
>> >> >> >> index 0000000..037f273
>> >> >> >> --- /dev/null
>> >> >> >> +++ b/drivers/spi/spi-renesas-rpc.c
>> >> >> > [...]
>> >> >> >> +static int rpc_spi_probe(struct platform_device *pdev)
>> >> >> >> +{
>> >> >> >> + struct spi_controller *ctlr;
>> >> >> >> + struct rpc_mfd *rpc_mfd = dev_get_drvdata(pdev->dev.parent);
>> >> >> >> + struct rpc_spi *rpc;
>> >> >> >> + int ret;
>> >> >> >> +
>> >> >> >> + ctlr = spi_alloc_master(&pdev->dev, sizeof(*rpc));
>> >> >> >> + if (!ctlr)
>> >> >> >> + return -ENOMEM;
>> >> >> >> +
>> >> >> >> + platform_set_drvdata(pdev, ctlr);
>> >> >> >> +
>> >> >> >> + rpc = spi_controller_get_devdata(ctlr);
>> >> >> >> +
>> >> >> >> + ctlr->dev.of_node = pdev->dev.of_node;
>> >> >> > [...]
>> >> >> >> +
>> >> >> >> + pm_runtime_enable(&pdev->dev);
>> >> >> >> + ctlr->auto_runtime_pm = true;
>> >> >> >
>> >> >> > I think this line no longer works as expected with the new
>> >> >> probing scheme.
>> >>
>> >> That's because we added another (SPI) device under our MFD.
>> >
>> > Do you mean just to remove one line
>> > ctlr->auto_runtime_pm = true;
>> > ?
>
> how did you test it ?
> what is your testing flow ?
>
>> No, you should explicitly call RPM for the MFD (not the SPI device).
>
> okay, patch RPM to RPC MFD and will remove SPI RPM enable part.
>
>>
>> >> >> > Have you tested reading? v8 patch still works while v9 patches
>> >> >> > hang on doing:
>> >> >> >
>> >> >> > $ cat /dev/mtd<n>...
>> >> >>
>> >> >> Sorry, 'od -x', not 'cat'.
>> >> >
>> >> > root@draak:/# cat /proc/mtd
>> >> > dev: size erasesize name
>> >> > mtd0: 00040000 00001000 "Bank 1 - Boot parameter"
>> >> > mtd1: 00140000 00001000 "Bank 1 - Loader-BL2"
>> >> > mtd2: 00040000 00001000 "Bank 1 - Certification"
>> >> > mtd3: 00080000 00001000 "Bank 1 - ARM Trusted FW"
>> >> > mtd4: 00400000 00001000 "Bank 1 - Reserved-1"
>> >> > mtd5: 00300000 00001000 "Bank 1 - U-Boot"
>> >> > mtd6: 00200000 00001000 "Bank 1 - Reserved-2"
>> >> > mtd7: 00480000 00001000 "Bank 1 - Splash"
>> >> > mtd8: 00040000 00001000 "Bank 1 - Device Tree"
>> >> > root@draak:/# od -x /dev/mtd1
>> >> > 0000000 0000 d280 0001 d280 0002 d280 0003 d280
>> >> > 0000020 0004 d280 0005 d280 0006 d280 0007 d280
>> >> > 0000040 0008 d280 0009 d280 000a d280 000b d280
>> >> > 0000060 000c d280 000d d280 000e d280 000f d280
>> >> > 0000100 0010 d280 0011 d280 0012 d280 0013 d280
>> >> > 0000120 0014 d280 0015 d280 0016 d280 0017 d280
>> >> > 0000140 0018 d280 0019 d280 001a d280 001b d280
>> >> > 0000160 001c d280 001d d280 001e d280 1000 d53e
>> >> > 0000200 f800 9266 1000 d51e 3fdf d503 3ba0 1005
>> >>
>> >> Still hangs for me. After I patches spi-mem.c and the driver to
>> >> call RPM for the MFD, it started working again. Perhaps, that clock>> >> is still enabled on your target. What does the following print (for

Even with these issues worked around, I still see strange behavior on
writes, e.g. after I mount JFFS2 partition, remove 1 file, unmount, re-mount,
and mount again, the removed file is back! :-/

[...]

> thanks & best regards,
> Mason

MBR, Sergei

2019-05-14 19:14:43

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver

On 05/14/2019 10:06 PM, Sergei Shtylyov wrote:

[...]
>>>>>>>> Have you tested reading? v8 patch still works while v9 patches
>>>>>>>> hang on doing:
>>>>>>>>
>>>>>>>> $ cat /dev/mtd<n>...
>>>>>>>
>>>>>>> Sorry, 'od -x', not 'cat'.
>>>>>>
>>>>>> root@draak:/# cat /proc/mtd
>>>>>> dev: size erasesize name
>>>>>> mtd0: 00040000 00001000 "Bank 1 - Boot parameter"
>>>>>> mtd1: 00140000 00001000 "Bank 1 - Loader-BL2"
>>>>>> mtd2: 00040000 00001000 "Bank 1 - Certification"
>>>>>> mtd3: 00080000 00001000 "Bank 1 - ARM Trusted FW"
>>>>>> mtd4: 00400000 00001000 "Bank 1 - Reserved-1"
>>>>>> mtd5: 00300000 00001000 "Bank 1 - U-Boot"
>>>>>> mtd6: 00200000 00001000 "Bank 1 - Reserved-2"
>>>>>> mtd7: 00480000 00001000 "Bank 1 - Splash"
>>>>>> mtd8: 00040000 00001000 "Bank 1 - Device Tree"
>>>>>> root@draak:/# od -x /dev/mtd1
>>>>>> 0000000 0000 d280 0001 d280 0002 d280 0003 d280
>>>>>> 0000020 0004 d280 0005 d280 0006 d280 0007 d280
>>>>>> 0000040 0008 d280 0009 d280 000a d280 000b d280
>>>>>> 0000060 000c d280 000d d280 000e d280 000f d280
>>>>>> 0000100 0010 d280 0011 d280 0012 d280 0013 d280
>>>>>> 0000120 0014 d280 0015 d280 0016 d280 0017 d280
>>>>>> 0000140 0018 d280 0019 d280 001a d280 001b d280
>>>>>> 0000160 001c d280 001d d280 001e d280 1000 d53e
>>>>>> 0000200 f800 9266 1000 d51e 3fdf d503 3ba0 1005
>>>>>
>>>>> Still hangs for me. After I patches spi-mem.c and the driver to
>>>>> call RPM for the MFD, it started working again. Perhaps, that clock>> >> is still enabled on your target. What does the following print (for
>
> Even with these issues worked around, I still see strange behavior on
> writes, e.g. after I mount JFFS2 partition, remove 1 file, unmount, re-mount,
> and mount again, the removed file is back! :-/

Too many re-mounts, one was enough. :-)

> [...]
>
>> thanks & best regards,
>> Mason

MBR, Sergei