[PATCH v4 0/2] spi: Add Renesas R-Car Gen3 RPC SPI driver

[PATCH v4 0/2] spi: Add Renesas R-Car Gen3 RPC SPI driver

[Mason Yang]

Hi Mark,

This Renesas R-Car Gen3 RPC SPI driver is based on Boris's new
spi-mem direct mapping read/write mode [1][2].

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

[1] https://patchwork.kernel.org/patch/10670753
[2] https://patchwork.kernel.org/patch/10670747


Mason Yang (2):
  spi: Add Renesas R-Car Gen3 RPC SPI controller driver
  dt-bindings: spi: Document Renesas R-Car Gen3 RPC controller bindings

 .../devicetree/bindings/spi/spi-renesas-rpc.txt    |  37 +
 drivers/spi/Kconfig                                |   6 +
 drivers/spi/Makefile                               |   1 +
 drivers/spi/spi-renesas-rpc.c                      | 788 +++++++++++++++++++++
 4 files changed, 832 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
 create mode 100644 drivers/spi/spi-renesas-rpc.c

-- 
1.9.1

[PATCH v4 1/2] spi: Add Renesas R-Car Gen3 RPC SPI controller driver

[Mason Yang]

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

Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
---
 drivers/spi/Kconfig           |   6 +
 drivers/spi/Makefile          |   1 +
 drivers/spi/spi-renesas-rpc.c | 788 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 795 insertions(+)
 create mode 100644 drivers/spi/spi-renesas-rpc.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 7d3a5c9..54b40f8 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -528,6 +528,12 @@ config SPI_RSPI
 	help
 	  SPI driver for Renesas RSPI and QSPI blocks.
 
+config SPI_RENESAS_RPC
+	tristate "Renesas R-Car Gen3 RPC SPI controller"
+	depends on ARCH_RENESAS || COMPILE_TEST
+	help
+	  SPI driver for Renesas R-Car Gen3 RPC.
+
 config SPI_QCOM_QSPI
 	tristate "QTI QSPI controller"
 	depends on ARCH_QCOM
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 3575205..5d5c523 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -81,6 +81,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..6dd739a
--- /dev/null
+++ b/drivers/spi/spi-renesas-rpc.c
@@ -0,0 +1,788 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (C) 2018 ~ 2019 Renesas Solutions Corp.
+// Copyright (C) 2018 Macronix International Co., Ltd.
+//
+// R-Car Gen3 RPC SPI/QSPI/Octa driver
+//
+// Authors:
+//	Mason Yang <masonccyang@mxic.com.tw>
+//
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/log2.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/spi-mem.h>
+
+#include <asm/unaligned.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)
+#define RPC_CMNCR_IO2FV(val)	(((val) & 0x3) << 12)
+#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_SPIDB(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) & 0x2) << 30)
+#define RPC_SMENR_OCDB(o)	(((o) & 0x2) << 28)
+#define RPC_SMENR_ADB(o)	(((o) & 0x2) << 24)
+#define RPC_SMENR_OPDB(o)	(((o) & 0x2) << 20)
+#define RPC_SMENR_SPIDB(o)	(((o) & 0x2) << 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	(0x5 << 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_PHYOFFSET2		0x0084	/* R/W */
+#define RPC_PHYOFFSET2_OCTTMG(v) (((v) & 0x7) << 8)
+
+#define RPC_WBUF		0x8000	/* Write Buffer */
+#define RPC_WBUF_SIZE		256	/* Write Buffer size */
+
+struct rpc_spi {
+	struct clk *clk_rpc;
+	void __iomem *base;
+	void __iomem *dirmap;
+	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 0x31511144 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, 0x31511144);
+	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));
+}
+
+static int rpc_spi_do_reset(struct rpc_spi *rpc)
+{
+	int ret;
+
+	ret = reset_control_reset(rpc->rstc);
+	if (ret)
+		return ret;
+
+	return 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 = 0;
+
+	regmap_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD | RPC_CMNCR_SFDE |
+				  RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
+				  RPC_CMNCR_BSZ(0));
+	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);
+
+	if (tx_buf) {
+		smenr = rpc->smenr;
+
+		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 out;
+
+			pos += nbytes;
+			smenr = rpc->smenr & ~RPC_SMENR_CDE &
+					     ~RPC_SMENR_ADE(0xf);
+		}
+	} else if (rx_buf) {
+		smenr = rpc->smenr;
+
+		while (pos < rpc->xferlen) {
+			u32 nbytes = rpc->xferlen - pos;
+
+			if (nbytes > 4)
+				nbytes = 4;
+
+			smcr = rpc->smcr | RPC_SMCR_SPIE;
+
+			if (rpc->xferlen > 4 && rpc->xferlen < 8 && pos == 0)
+				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 out;
+
+			regmap_read(rpc->regmap, RPC_SMRDR0, &data);
+			memcpy(rx_buf + pos, &data, nbytes);
+			pos += nbytes;
+
+			if (rpc->xferlen > 4 && rpc->xferlen < 8 && pos == 4) {
+				smenr = rpc->smenr & ~RPC_SMENR_CDE &
+					~RPC_SMENR_ADE(0xf);
+			} else {
+				regmap_write(rpc->regmap, RPC_SMCMR, rpc->cmd);
+				regmap_write(rpc->regmap, RPC_SMDMCR,
+					     rpc->dummy);
+				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 out;
+	}
+
+	return ret;
+
+out:
+	return rpc_spi_do_reset(rpc);
+}
+
+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_master_get_devdata(spi->master);
+
+	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)) {
+		if (op->data.dir == SPI_MEM_DATA_IN) {
+			rpc->smcr = RPC_SMCR_SPIRE;
+			rpc->xfer_dir = SPI_MEM_DATA_IN;
+		} else if (op->data.dir == SPI_MEM_DATA_OUT) {
+			rpc->smcr = RPC_SMCR_SPIWE;
+			rpc->xfer_dir = SPI_MEM_DATA_OUT;
+		}
+
+		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_master_get_devdata(desc->mem->spi->master);
+	int ret;
+
+	if (WARN_ON(offs + desc->info.offset + len > U32_MAX))
+		return -EINVAL;
+
+	if (WARN_ON(len > 0x4000000))
+		return -EIO;
+
+	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_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_SFDE |
+		     RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
+		     RPC_CMNCR_BSZ(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_master_get_devdata(desc->mem->spi->master);
+	int ret;
+
+	if (WARN_ON(offs + desc->info.offset + len > U32_MAX))
+		return -EINVAL;
+
+	if (WARN_ON(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_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD | RPC_CMNCR_SFDE |
+				  RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
+				  RPC_CMNCR_BSZ(0));
+	regmap_write(rpc->regmap, RPC_SMDRENR, 0);
+	regmap_write(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_CAL | 0x260 |
+				  RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF);
+
+	memcpy_toio(rpc->base + 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 out;
+
+	regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RCF);
+	regmap_write(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_CAL |
+				  RPC_PHYCNT_STRTIM(6) | 0x260);
+
+	return len;
+
+out:
+	return rpc_spi_do_reset(rpc);
+}
+
+static int rpc_spi_mem_dirmap_create(struct spi_mem_dirmap_desc *desc)
+{
+	struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
+
+	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;
+
+	return 0;
+}
+
+static int rpc_spi_mem_exec_op(struct spi_mem *mem,
+			       const struct spi_mem_op *op)
+{
+	struct rpc_spi *rpc = spi_master_get_devdata(mem->spi->master);
+	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 int rpc_spi_xfer_message(struct rpc_spi *rpc, struct spi_transfer *t)
+{
+	int ret;
+
+	ret = rpc_spi_set_freq(rpc, t->speed_hz);
+	if (ret)
+		return ret;
+
+	ret = rpc_spi_io_xfer(rpc,
+			      rpc->xfer_dir == SPI_MEM_DATA_OUT ?
+			      t->tx_buf : NULL,
+			      rpc->xfer_dir == SPI_MEM_DATA_IN ?
+			      t->rx_buf : NULL);
+
+	return ret;
+}
+
+static int rpc_spi_transfer_one_message(struct spi_master *master,
+					struct spi_message *msg)
+{
+	struct rpc_spi *rpc = spi_master_get_devdata(master);
+	struct spi_transfer *t;
+	int ret;
+
+	rpc_spi_transfer_setup(rpc, msg);
+
+	list_for_each_entry(t, &msg->transfers, transfer_list) {
+		if (!list_is_last(&t->transfer_list, &msg->transfers))
+			continue;
+		ret = rpc_spi_xfer_message(rpc, t);
+		if (ret)
+			goto out;
+	}
+
+	msg->status = 0;
+	msg->actual_length = rpc->totalxferlen;
+out:
+	spi_finalize_current_message(master);
+	return 0;
+}
+
+static const struct regmap_range rpc_spi_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_spi_volatile_table = {
+	.yes_ranges	= rpc_spi_volatile_ranges,
+	.n_yes_ranges	= ARRAY_SIZE(rpc_spi_volatile_ranges),
+};
+
+static const struct regmap_config rpc_spi_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.fast_io = true,
+	.max_register = RPC_PHYOFFSET2,
+	.volatile_table = &rpc_spi_volatile_table,
+};
+
+static int rpc_spi_probe(struct platform_device *pdev)
+{
+	struct spi_master *master;
+	struct resource *res;
+	struct rpc_spi *rpc;
+	const struct regmap_config *regmap_config;
+	const char *mode;
+	int ret;
+
+	ret = of_property_read_string(pdev->dev.of_node,
+				      "renesas,rpc-mode", &mode);
+	if (ret < 0)
+		return ret;
+
+	if (strcasecmp(mode, "spi"))
+		return -ENODEV;
+
+	master = spi_alloc_master(&pdev->dev, sizeof(*rpc));
+	if (!master)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, master);
+
+	rpc = spi_master_get_devdata(master);
+
+	master->dev.of_node = pdev->dev.of_node;
+
+	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);
+
+	regmap_config = &rpc_spi_regmap_config;
+	rpc->regmap = devm_regmap_init_mmio(&pdev->dev, rpc->base,
+					    regmap_config);
+	if (IS_ERR(rpc->regmap)) {
+		dev_err(&pdev->dev, "failed to init regmap %ld for rpc-spi\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;
+
+	rpc->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+	if (IS_ERR(rpc->rstc))
+		return PTR_ERR(rpc->rstc);
+
+	pm_runtime_enable(&pdev->dev);
+	master->auto_runtime_pm = true;
+
+	master->num_chipselect = 1;
+	master->mem_ops = &rpc_spi_mem_ops;
+	master->transfer_one_message = rpc_spi_transfer_one_message;
+
+	master->bits_per_word_mask = SPI_BPW_MASK(8);
+	master->mode_bits = SPI_CPOL | SPI_CPHA;
+
+	rpc_spi_hw_init(rpc);
+
+	ret = spi_register_master(master);
+	if (ret) {
+		dev_err(&pdev->dev, "spi_register_master failed\n");
+		goto err_put_master;
+	}
+	return 0;
+
+err_put_master:
+	spi_master_put(master);
+	pm_runtime_disable(&pdev->dev);
+
+	return ret;
+}
+
+static int rpc_spi_remove(struct platform_device *pdev)
+{
+	struct spi_master *master = platform_get_drvdata(pdev);
+
+	pm_runtime_disable(&pdev->dev);
+	spi_unregister_master(master);
+
+	return 0;
+}
+
+static const struct of_device_id rpc_spi_of_ids[] = {
+	{ .compatible = "renesas,r8a77995-rpc", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rpc_spi_of_ids);
+
+#ifdef CONFIG_PM_SLEEP
+static int rpc_spi_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct spi_master *master = platform_get_drvdata(pdev);
+
+	return spi_master_suspend(master);
+}
+
+static int rpc_spi_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct spi_master *master = platform_get_drvdata(pdev);
+
+	return spi_master_resume(master);
+}
+
+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 <masonccyang@mxic.com.tw>");
+MODULE_DESCRIPTION("Renesas R-Car Gen3 RPC SPI controller driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH v4 1/2] spi: Add Renesas R-Car Gen3 RPC SPI controller driver

[Sergei Shtylyov]

On 12/24/2018 09:52 AM, Mason Yang wrote:

> Add a driver for Renesas R-Car Gen3 RPC SPI controller.
> 
> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> ---
>  drivers/spi/Kconfig           |   6 +
>  drivers/spi/Makefile          |   1 +
>  drivers/spi/spi-renesas-rpc.c | 788 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 795 insertions(+)
>  create mode 100644 drivers/spi/spi-renesas-rpc.c
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 7d3a5c9..54b40f8 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -528,6 +528,12 @@ config SPI_RSPI
>  	help
>  	  SPI driver for Renesas RSPI and QSPI blocks.
>  
> +config SPI_RENESAS_RPC
> +	tristate "Renesas R-Car Gen3 RPC SPI controller"

   Well, technically it's called RPC-IF in the manual, not just RPC...

> +	depends on ARCH_RENESAS || COMPILE_TEST
> +	help
> +	  SPI driver for Renesas R-Car Gen3 RPC.
> +
>  config SPI_QCOM_QSPI
>  	tristate "QTI QSPI controller"
>  	depends on ARCH_QCOM
[...]
> diff --git a/drivers/spi/spi-renesas-rpc.c b/drivers/spi/spi-renesas-rpc.c
> new file mode 100644
> index 0000000..6dd739a
> --- /dev/null
> +++ b/drivers/spi/spi-renesas-rpc.c
> @@ -0,0 +1,788 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (C) 2018 ~ 2019 Renesas Solutions Corp.
> +// Copyright (C) 2018 Macronix International Co., Ltd.
> +//
> +// R-Car Gen3 RPC SPI/QSPI/Octa driver
> +//
> +// Authors:
> +//	Mason Yang <masonccyang@mxic.com.tw>
> +//
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/log2.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spi-mem.h>
> +
> +#include <asm/unaligned.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)
> +#define RPC_CMNCR_IO2FV(val)	(((val) & 0x3) << 12)

   Like I said, the above 2 aren't documented in the manual v1.00...

> +#define RPC_CMNCR_IO0FV(val)	(((val) & 0x3) << 8)

   Only this one is...

[...]
> +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 0x31511144 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, 0x31511144);

  0x30000000 is documented, missed that last time...

[...]
> +static int rpc_spi_do_reset(struct rpc_spi *rpc)
> +{
> +	int ret;
> +
> +	ret = reset_control_reset(rpc->rstc);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

   Like I said, this function folds to a mere reset_control_reset() call...

[...]
> +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 = 0;

   Looks like we don't need this variable...

> +
> +	regmap_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD | RPC_CMNCR_SFDE |
> +				  RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
> +				  RPC_CMNCR_BSZ(0));
> +	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);
[...]
> +	} else if (rx_buf) {
> +		smenr = rpc->smenr;

   Could be done before the *if*...

> +
> +		while (pos < rpc->xferlen) {
> +			u32 nbytes = rpc->xferlen - pos;
> +
> +			if (nbytes > 4)
> +				nbytes = 4;
> +
> +			smcr = rpc->smcr | RPC_SMCR_SPIE;
> +
> +			if (rpc->xferlen > 4 && rpc->xferlen < 8 && pos == 0)
> +				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 out;
> +
> +			regmap_read(rpc->regmap, RPC_SMRDR0, &data);
> +			memcpy(rx_buf + pos, &data, nbytes);
> +			pos += nbytes;
> +
> +			if (rpc->xferlen > 4 && rpc->xferlen < 8 && pos == 4) {

   This looks hackish. What I think matters is whether the address bits are set or not.
Anyway, maybe it works OK for you but not for me (on V3H), the 4th byte of the JEDEC ID
is clobbered from 0 to 3... I've been working on a better workaround using Marek's
approach (reading in extended memory mode) -- should port to v4 of your patch yet...

> +				smenr = rpc->smenr & ~RPC_SMENR_CDE &
> +					~RPC_SMENR_ADE(0xf);
> +			} else {
> +				regmap_write(rpc->regmap, RPC_SMCMR, rpc->cmd);
> +				regmap_write(rpc->regmap, RPC_SMDMCR,
> +					     rpc->dummy);

   Not sure why you rewrite these regs again. Where do they change?

> +				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 out;
> +	}
> +
> +	return ret;

   Could be *return* 0, we never get here from an error path...

> +
> +out:
> +	return rpc_spi_do_reset(rpc);
> +}
> +
> +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_master_get_devdata(spi->master);
> +
> +	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)) {
> +		if (op->data.dir == SPI_MEM_DATA_IN) {
> +			rpc->smcr = RPC_SMCR_SPIRE;
> +			rpc->xfer_dir = SPI_MEM_DATA_IN;
> +		} else if (op->data.dir == SPI_MEM_DATA_OUT) {
> +			rpc->smcr = RPC_SMCR_SPIWE;
> +			rpc->xfer_dir = SPI_MEM_DATA_OUT;
> +		}

   Use *switch* instead, please.

[...]
> +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;

   So we support the dual mode, even though the manual doesn't say we do?

> +
> +	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_master_get_devdata(desc->mem->spi->master);
> +	int ret;
> +
> +	if (WARN_ON(offs + desc->info.offset + len > U32_MAX))
> +		return -EINVAL;
> +
> +	if (WARN_ON(len > 0x4000000))
> +		return -EIO;

   Why not read what can still be read and return 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_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_SFDE |
> +		     RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
> +		     RPC_CMNCR_BSZ(0));

   Why not set this in the probing time and only set/clear the MD bit?

[...]
> +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_master_get_devdata(desc->mem->spi->master);
> +	int ret;
> +
> +	if (WARN_ON(offs + desc->info.offset + len > U32_MAX))
> +		return -EINVAL;
> +
> +	if (WARN_ON(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_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD | RPC_CMNCR_SFDE |
> +				  RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
> +				  RPC_CMNCR_BSZ(0));
> +	regmap_write(rpc->regmap, RPC_SMDRENR, 0);
> +	regmap_write(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_CAL | 0x260 |
> +				  RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF);
> +
> +	memcpy_toio(rpc->base + 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 out;
> +
> +	regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RCF);
> +	regmap_write(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_CAL |
> +				  RPC_PHYCNT_STRTIM(6) | 0x260);

   Whe not do read-modify-write here and above?

[...]
> +static void rpc_spi_transfer_setup(struct rpc_spi *rpc,
> +				   struct spi_message *msg)
> +{
[...]
> +	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));

   Mhm, I would indent this contination line by 1 extra tab...

> +		} 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));

   And this one...

[...]
> +#ifdef CONFIG_PM_SLEEP
> +static int rpc_spi_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct spi_master *master = platform_get_drvdata(pdev);

   Ugh... not sure how i missed it the last time. :-/ Please, just do this:

	struct spi_master *master = dev_get_drvdata(dev);

> +
> +	return spi_master_suspend(master);
> +}
> +
> +static int rpc_spi_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct spi_master *master = platform_get_drvdata(pdev);

   Likewise...

[...]

MBR, Sergei

Re: [PATCH v4 1/2] spi: Add Renesas R-Car Gen3 RPC SPI controller driver

[Sergei Shtylyov]

Hello!

On 12/26/2018 07:24 AM, masonccyang@mxic.com.tw wrote:

>> [...]
>> > diff --git a/drivers/spi/spi-renesas-rpc.c b/drivers/spi/spi-renesas-rpc.c
>> > new file mode 100644
>> > index 0000000..6dd739a
>> > --- /dev/null
>> > +++ b/drivers/spi/spi-renesas-rpc.c
>> > @@ -0,0 +1,788 @@
[...]
>> > +#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)
>> > +#define RPC_CMNCR_IO2FV(val)   (((val) & 0x3) << 12)
>>
>>    Like I said, the above 2 aren't documented in the manual v1.00...
> 
> okay, add a description as:
> /* RPC_CMNCR_IO3FV/IO2FV are undocumented bit, but must be set */
> #define RPC_CMNCR_IO3FV(val)    (((val) & 0x3) << 14)
> #define RPC_CMNCR_IO2FV(val)    (((val) & 0x3) << 12)
> #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))
> 
> is it ok?

   Yes. But would have been enough if you just commented with // on the same line --
it seems these are legal now... 

>> [...]
>> > +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 0x31511144 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, 0x31511144);
>>
>>   0x30000000 is documented, missed that last time...
>>
> 
> okay,patch it to:
> 
> #define RPC_PHYOFFSET1_DDRTMG(v) (((v) & 0x3) << 28)
> 
> regmap_write(rpc->regmap, RPC_PHYOFFSET1,
>                           RPC_PHYOFFSET1_DDRTMG(3) | 0x1511144);
> 

   OK, thanx.

>> [...]
>> > +static int rpc_spi_do_reset(struct rpc_spi *rpc)
>> > +{
>> > +   int ret;
>> > +
>> > +   ret = reset_control_reset(rpc->rstc);
>> > +   if (ret)
>> > +      return ret;
>> > +
>> > +   return 0;
>> > +}
>>
>>    Like I said, this function folds to a mere reset_control_reset() call...
>>
> 
> Do you mean just drop this rpc_spi_do_reset( )

   I mean we don't need this wrapper, we can call reset_contreol_reset() directly.

> because driver is never
> come here from an error path ?

   You are mixing things up -- of course we call it from the error path.

>> [...]
>> > +
>> > +      while (pos < rpc->xferlen) {
>> > +         u32 nbytes = rpc->xferlen - pos;
>> > +
>> > +         if (nbytes > 4)
>> > +            nbytes = 4;
>> > +
>> > +         smcr = rpc->smcr | RPC_SMCR_SPIE;
>> > +
>> > +         if (rpc->xferlen > 4 && rpc->xferlen < 8 && pos == 0)
>> > +            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 out;
>> > +
>> > +         regmap_read(rpc->regmap, RPC_SMRDR0, &data);
>> > +         memcpy(rx_buf + pos, &data, nbytes);
>> > +         pos += nbytes;
>> > +
>> > +         if (rpc->xferlen > 4 && rpc->xferlen < 8 && pos == 4) {
>>
>>    This looks hackish. What I think matters is whether the address
>> bits are set or not.
>> Anyway, maybe it works OK for you but not for me (on V3H), the 4th
>> byte of the JEDEC ID
>> is clobbered from 0 to 3... I've been working on a better workaround
>> using Marek's
>> approach (reading in extended memory mode) -- should port to v4 of
>> your patch yet...
> 
> Do you mean I also patch external address space read mode
> for RPC to read ID and data ?

   No, I meant using the dirmap read mode for RDID and company. I have the patch
almost ready now and I hope you'll merge it with yours... either that or add it to
your series atop of this patch.

[...]
> I also think this is kind of RPC HW bug in manual I/O mode.

   Yes.

> Renesas FAE@Taiwan has replied me that their the last bare-metal code,
> mini-monitor v5.x still use one command to read 4 bytes data each time
> and I think RPC HW designer should have known this HW bug already.

   Have they tried it on V3H?

>> > +            smenr = rpc->smenr & ~RPC_SMENR_CDE &
>> > +               ~RPC_SMENR_ADE(0xf);
>> > +         } else {
>> > +            regmap_write(rpc->regmap, RPC_SMCMR, rpc->cmd);
>> > +            regmap_write(rpc->regmap, RPC_SMDMCR,
>> > +                    rpc->dummy);
>>
>>    Not sure why you rewrite these regs again. Where do they change?
> 
> Make sure the value in these register are setting correctly
> before RPC starting a SPI transfer.

   The are -- ath the start of this function.

> Not sure RPC HW behavior will change these registers after a transfer.

   I doubt it.

> In RPC bare-metal code mini-monitor v4.01 also do this way.

   Well, we shouldn't blindly copy wgat they did, I think.

>> > +            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 out;
>> > +   }
>> > +
>> > +   return ret;
>>
>>    Could be *return* 0, we never get here from an error path...
> 
> see above!

   You've mixed things up. We always come here with ret == 0.

>> > +
>> > +out:
>> > +   return rpc_spi_do_reset(rpc);
>> > +}
>> > +
>> > +static void rpc_spi_mem_set_prep_op_cfg(struct spi_device *spi,
>> > +               const struct spi_mem_op *op,
>> > +               u64 *offs, size_t *len)
[...]
>> > +   if (op->data.nbytes || (offs && len)) {
>> > +      if (op->data.dir == SPI_MEM_DATA_IN) {
>> > +         rpc->smcr = RPC_SMCR_SPIRE;
>> > +         rpc->xfer_dir = SPI_MEM_DATA_IN;
>> > +      } else if (op->data.dir == SPI_MEM_DATA_OUT) {
>> > +         rpc->smcr = RPC_SMCR_SPIWE;
>> > +         rpc->xfer_dir = SPI_MEM_DATA_OUT;
>> > +      }
>>
>>    Use *switch* instead, please.
>>
> 
> why ?
> only two condition here!

   Oh, so you have some "threshold" on when to use *switch*? :-)
   I think each time we compare the same varible with a constant 2+
times, we need to use *switch*.

>> [...]
>> > +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;
>>
>>    So we support the dual mode, even though the manual doesn't say we do?
> 
> I think driver would never go to dual mode by setting SPI master->mode_bits.

   Maybe. BTW, when I test the driver in the renesas.git devel branch, I get:

spi spi0.0: setup: ignoring unsupported mode bits 800

[...]
>> > +   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_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_SFDE |
>> > +           RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
>> > +           RPC_CMNCR_BSZ(0));
>>
>>    Why not set this in the probing time and only set/clear the MD bit?
>>
> 
> same above!
> Make sure the value in these register are setting correctly
> before RPC starting a SPI transfer.

   You can set it once and only change the bits you need to change afterwards.
What's wrong with it?

>> [...]
>> > +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_master_get_devdata(desc->mem->spi->master);
>> > +   int ret;
>> > +
>> > +   if (WARN_ON(offs + desc->info.offset + len > U32_MAX))
>> > +      return -EINVAL;
>> > +
>> > +   if (WARN_ON(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_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD | RPC_CMNCR_SFDE |
>> > +              RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
>> > +              RPC_CMNCR_BSZ(0));
>> > +   regmap_write(rpc->regmap, RPC_SMDRENR, 0);
>> > +   regmap_write(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_CAL | 0x260 |
>> > +              RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF);
>> > +
>> > +   memcpy_toio(rpc->base + 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 out;
>> > +
>> > +   regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RCF);
>> > +   regmap_write(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_CAL |
>> > +              RPC_PHYCNT_STRTIM(6) | 0x260);
>>
>>    Whe not do read-modify-write here and above?
> 
> same above!
> Make sure the value in these register are setting correctly
> before RPC starting a SPI transfer.

   Nobody can spoil the register values with yours being a single driver controlling
it, no?

>> [...]
>> > +static void rpc_spi_transfer_setup(struct rpc_spi *rpc,
>> > +               struct spi_message *msg)
>> > +{
>> [...]
>> > +   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));
>>
>>    Mhm, I would indent this contination line by 1 extra tab...
>>
>> > +      } 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));
>>
>>    And this one...
> 
> like this ?
> --------------------------------------------------------------------
>          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));

   I didn't mean you need to leave ( on the first line, can be left on the  new
line, as before.

>> [...]

> thanks for your review.
> best regards,
> Mason
[...]

MBR, Sergei

Re: [PATCH v4 1/2] spi: Add Renesas R-Car Gen3 RPC SPI controller driver

[Sergei Shtylyov]

Hello!

On 01/03/2019 09:35 AM, masonccyang@mxic.com.tw wrote:

[...]
>> >> > +#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)
>> >> > +#define RPC_CMNCR_IO2FV(val)   (((val) & 0x3) << 12)
>> >>
>> >>    Like I said, the above 2 aren't documented in the manual v1.00...
>> >
>> > okay, add a description as:
>> > /* RPC_CMNCR_IO3FV/IO2FV are undocumented bit, but must be set */
>> > #define RPC_CMNCR_IO3FV(val)    (((val) & 0x3) << 14)
>> > #define RPC_CMNCR_IO2FV(val)    (((val) & 0x3) << 12)
>> > #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))
>> >
>> > is it ok?
>>
>>    Yes. But would have been enough if you just commented with // on
>> the same line --
>> it seems these are legal now...
> 
> on the same line is over 80 char,
> #define RPC_CMNCR_IO3FV(val)    (((val) & 0x3) << 14) // undocumented bit, but must be set
> #define RPC_CMNCR_IO2FV(val)    (((val) & 0x3) << 12) // undocumented bit, but must be set
> 
> or just
> #define RPC_CMNCR_IO3FV(val)    (((val) & 0x3) << 14) // undocumented bit
> #define RPC_CMNCR_IO2FV(val)    (((val) & 0x3) << 12) // undocumented bit
> is it ok ?

   The second variant would be enough.

[...]

>> >> > +   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_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_SFDE |
>> >> > +           RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
>> >> > +           RPC_CMNCR_BSZ(0));
>> >>
>> >>    Why not set this in the probing time and only set/clear the MD bit?
>> >>
>> >
>> > same above!
>> > Make sure the value in these register are setting correctly
>> > before RPC starting a SPI transfer.
>>
>>    You can set it once and only change the bits you need to change afterwards.
>> What's wrong with it?
>>
> 
> if so, it will patch to:
> ------------------------------------------------------
> regmap_read(rpc->regmap, RPC_CMNCR, &data);
> data &= ~RPC_CMNCR_MD;
> regmap_write(rpc->regmap, RPC_CMNCR, data);
> ------------------------------------------------------
> Do you think this way is better ?

    No, this one is better:

	regmap_update_bits(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD, 0);

> maybe this is better,
> write(read(rpc->regs + RPC_CMNCR) & ~RPC_CMNCR_MD,
>         rpc->regs + RPC_CMNCR);

   It's effectively the same code as your 1st variant...
 
[...]
>> >> > +static void rpc_spi_transfer_setup(struct rpc_spi *rpc,
>> >> > +               struct spi_message *msg)
>> >> > +{
>> >> [...]
>> >> > +   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));
>> >>
>> >>    Mhm, I would indent this contination line by 1 extra tab...
>> >>
>> >> > +      } 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));
>> >>
>> >>    And this one...
>> >
>> > like this ?
>> > --------------------------------------------------------------------
>> >          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));
>>
>>    I didn't mean you need to leave ( on the first line, can be left
>> on the  new
>> line, as before.
>>
> 
> how about this style ?
> -------------------------------------------------------------------------------------
>          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));
>                  }
>          }

   Looks even worse...

> ------------------------------------------------------------------------------------------
> 
> best regards,
> Mason

[...]

MBR, Sergei

[PATCH v4 2/2] dt-bindings: spi: Document Renesas R-Car Gen3 RPC controller bindings

[Mason Yang]

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

Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
---
 .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 37 ++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt

diff --git a/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
new file mode 100644
index 0000000..ba863d7
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
@@ -0,0 +1,37 @@
+Renesas R-Car Gen3 RPC controller Device Tree Bindings
+------------------------------------------------------
+
+Required properties:
+- compatible: should be "renesas,r8a77995-rpc"
+- #address-cells: should be 1
+- #size-cells: should be 0
+- reg: should contain 2 entries, one for the registers and one for the direct
+       mapping area
+- reg-names: should contain "regs" and "dirmap"
+- clock-names: should contain "rpc"
+- clocks: should contain 1 entries for the module's clock
+- renesas,rpc-mode: should contain "spi" for rpc spi mode or
+			   	   "hyperflash" for rpc hyerflash mode.
+
+Example:
+
+	rpc: rpc@ee200000 {
+		compatible = "renesas,r8a77995-rpc";
+		reg = <0 0xee200000 0 0x8100>, <0 0x08000000 0 0x4000000>;
+		reg-names = "regs", "dirmap";
+		clocks = <&cpg CPG_MOD 917>;
+		power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;
+		resets = <&cpg 917>;
+		clock-names = "rpc";
+		renesas,rpc-mode = "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

Re: [PATCH v4 2/2] dt-bindings: spi: Document Renesas R-Car Gen3 RPC controller bindings

[Sergei Shtylyov]

Hello!

On 24.12.2018 9:52, Mason Yang wrote:

> Document the bindings used by the Renesas R-Car Gen3 RPC controller.
> 
> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> ---
>   .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 37 ++++++++++++++++++++++
>   1 file changed, 37 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> new file mode 100644
> index 0000000..ba863d7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> @@ -0,0 +1,37 @@
> +Renesas R-Car Gen3 RPC controller Device Tree Bindings
> +------------------------------------------------------
> +
> +Required properties:
> +- compatible: should be "renesas,r8a77995-rpc"
> +- #address-cells: should be 1
> +- #size-cells: should be 0
> +- reg: should contain 2 entries, one for the registers and one for the direct
> +       mapping area
> +- reg-names: should contain "regs" and "dirmap"
> +- clock-names: should contain "rpc"
> +- clocks: should contain 1 entries for the module's clock
> +- renesas,rpc-mode: should contain "spi" for rpc spi mode or
> +			   	   "hyperflash" for rpc hyerflash mode.
                                                           ^ p

[...]

MBR, Sergei

Re: [PATCH v4 2/2] dt-bindings: spi: Document Renesas R-Car Gen3 RPC controller bindings

[Marek Vasut]

On 12/24/18 7:52 AM, Mason Yang wrote:
> Document the bindings used by the Renesas R-Car Gen3 RPC controller.
> 
> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> ---
>  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 37 ++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> new file mode 100644
> index 0000000..ba863d7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> @@ -0,0 +1,37 @@
> +Renesas R-Car Gen3 RPC controller Device Tree Bindings
> +------------------------------------------------------
> +
> +Required properties:
> +- compatible: should be "renesas,r8a77995-rpc"
> +- #address-cells: should be 1
> +- #size-cells: should be 0
> +- reg: should contain 2 entries, one for the registers and one for the direct
> +       mapping area
> +- reg-names: should contain "regs" and "dirmap"
> +- clock-names: should contain "rpc"
> +- clocks: should contain 1 entries for the module's clock
> +- renesas,rpc-mode: should contain "spi" for rpc spi mode or
> +			   	   "hyperflash" for rpc hyerflash mode.

We do not need special property to identify that the controller is in HF
mode, just attach CFI NOR node or JEDEC SPI NOR underneath it.

-- 
Best regards,
Marek Vasut

Re: [PATCH v4 0/2] spi: Add Renesas R-Car Gen3 RPC SPI driver

[Marek Vasut]

On 12/24/18 7:52 AM, Mason Yang wrote:
> Hi Mark,
> 
> This Renesas R-Car Gen3 RPC SPI driver is based on Boris's new
> spi-mem direct mapping read/write mode [1][2].

Again, the RPC is NOT a SPI controller, it is dual SPI/HF controller.

[...]

-- 
Best regards,
Marek Vasut

Re: [PATCH v4 0/2] spi: Add Renesas R-Car Gen3 RPC SPI driver

[Sergei Shtylyov]

On 12/24/2018 09:52 AM, Mason Yang wrote:

> Hi Mark,
> This Renesas R-Car Gen3 RPC SPI driver is based on Boris's new
> spi-mem direct mapping read/write mode [1][2].

   Against which repo/branch these pathes were generated?

MBR, Sergei