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

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

[Mason Yang]

Hi Mark,

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

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 RPC SPI controller driver
  dt-binding: spi: Document Renesas R-Car RPC controller bindings

 .../devicetree/bindings/spi/spi-renesas-rpc.txt    |  33 +
 drivers/spi/Kconfig                                |   6 +
 drivers/spi/Makefile                               |   1 +
 drivers/spi/spi-renesas-rpc.c                      | 750 +++++++++++++++++++++
 4 files changed, 790 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 1/2] spi: Add Renesas R-Car RPC SPI controller driver

[Mason Yang]

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

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

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 7d3a5c9..093006a 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 D3 RPC SPI controller"
+	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
+	help
+	  SPI driver for Renesas R-Car D3 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..00b9d8f
--- /dev/null
+++ b/drivers/spi/spi-renesas-rpc.c
@@ -0,0 +1,750 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (C) 2018 ~ 2019 Renesas Solutions Corp.
+// Copyright (C) 2018 Macronix International Co., Ltd.
+//
+// R-Car D3 RPC SPI/QSPI/Octa driver
+//
+// Authors:
+//	Mason Yang <masonccyang@mxic.com.tw>
+//
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/spi-mem.h>
+
+#define RPC_CMNCR		0x0000	/* R/W */
+#define RPC_CMNCR_MD		BIT(31)
+#define RPC_CMNCR_SFDE		BIT(24)
+#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_CPHAT		BIT(6)
+#define RPC_CMNCR_CPHAR		BIT(5)
+#define RPC_CMNCR_SSLP		BIT(4)
+#define RPC_CMNCR_CPOL		BIT(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) & 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		BIT(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_OPD0(o)	(((o) & 0xFF) << 0)
+#define RPC_SMOPR_OPD1(o)	(((o) & 0xFF) << 8)
+#define RPC_SMOPR_OPD2(o)	(((o) & 0xFF) << 16)
+#define RPC_SMOPR_OPD3(o)	(((o) & 0xFF) << 24)
+
+#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_MEM(v)	(((v) & 0x3) << 0)
+
+#define RPC_PHYOFFSET1		0x0080	/* R/W */
+#define RPC_PHYOFFSET2		0x0084	/* R/W */
+
+#define RPC_WBUF		0x8000	/* Write Buffer */
+#define RPC_WBUF_SIZE		256	/* Write Buffer size */
+
+struct rpc_spi {
+	struct clk *clk_rpc;
+	void __iomem *regs;
+	struct {
+		void __iomem *map;
+		dma_addr_t dma;
+		size_t size;
+	} linear;
+	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;
+};
+
+static int rpc_spi_set_freq(struct rpc_spi *rpc, unsigned long freq)
+{
+	int ret;
+
+	if (rpc->cur_speed_hz == freq)
+		return 0;
+
+	clk_disable_unprepare(rpc->clk_rpc);
+	ret = clk_set_rate(rpc->clk_rpc, freq);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(rpc->clk_rpc);
+	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.
+	 */
+	writel(RPC_PHYCNT_CAL | RPC_PHYCNT_STRTIM(0x3) | 0x260,
+	       rpc->regs + RPC_PHYCNT);
+
+	/*
+	 * NOTE: The 0x31511144 and 0x431 are undocumented bits,
+	 *	 but they must be set for RPC_PHYOFFSET1 & RPC_PHYOFFSET2.
+	 */
+	writel(0x31511144, rpc->regs + RPC_PHYOFFSET1);
+	writel(0x431, rpc->regs + RPC_PHYOFFSET2);
+
+	writel(RPC_SSLDR_SPNDL(7) | RPC_SSLDR_SLNDL(7) |
+	       RPC_SSLDR_SCKDL(7), rpc->regs + RPC_SSLDR);
+}
+
+static int wait_msg_xfer_end(struct rpc_spi *rpc)
+{
+	u32 sts;
+
+	return readl_poll_timeout(rpc->regs + RPC_CMNSR, sts,
+				  sts & RPC_CMNSR_TEND, 0, USEC_PER_SEC);
+}
+
+static u8 rpc_bits_xfer(u32 nbytes)
+{
+	u8 databyte;
+
+	switch (nbytes) {
+	case 1:
+		databyte = 0x8;
+		break;
+	case 2:
+		databyte = 0xc;
+		break;
+	default:
+		databyte = 0xf;
+		break;
+	}
+
+	return databyte;
+}
+
+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;
+
+	writel(RPC_CMNCR_MD | RPC_CMNCR_SFDE | RPC_CMNCR_MOIIO_HIZ |
+	       RPC_CMNCR_IOFV_HIZ | RPC_CMNCR_BSZ(0), rpc->regs + RPC_CMNCR);
+	writel(0x0, rpc->regs + RPC_SMDRENR);
+
+	if (tx_buf) {
+		writel(rpc->cmd, rpc->regs + RPC_SMCMR);
+		writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
+		writel(rpc->addr, rpc->regs + RPC_SMADR);
+		smenr = rpc->smenr;
+
+		while (pos < rpc->xferlen) {
+			u32 nbytes = rpc->xferlen  - pos;
+
+			writel(*(u32 *)(tx_buf + pos), rpc->regs + RPC_SMWDR0);
+
+			if (nbytes > 4) {
+				nbytes = 4;
+				smcr = rpc->smcr |
+				       RPC_SMCR_SPIE | RPC_SMCR_SSLKP;
+			} else {
+				smcr = rpc->smcr | RPC_SMCR_SPIE;
+			}
+
+			writel(smenr, rpc->regs + RPC_SMENR);
+			writel(smcr, rpc->regs + RPC_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) {
+		while (pos < rpc->xferlen) {
+			u32 nbytes = rpc->xferlen  - pos;
+
+			if (nbytes > 4)
+				nbytes = 4;
+
+			writel(rpc->cmd, rpc->regs + RPC_SMCMR);
+			writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
+			writel(rpc->addr + pos, rpc->regs + RPC_SMADR);
+			writel(rpc->smenr, rpc->regs + RPC_SMENR);
+			writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
+			ret = wait_msg_xfer_end(rpc);
+			if (ret)
+				goto out;
+
+			data = readl(rpc->regs + RPC_SMRDR0);
+			memcpy_fromio(rx_buf + pos, (void *)&data, nbytes);
+			pos += nbytes;
+		}
+	} else {
+		writel(rpc->cmd, rpc->regs + RPC_SMCMR);
+		writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
+		writel(rpc->addr + pos, rpc->regs + RPC_SMADR);
+		writel(rpc->smenr, rpc->regs + RPC_SMENR);
+		writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
+		ret = wait_msg_xfer_end(rpc);
+	}
+out:
+	return ret;
+}
+
+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(fls(op->cmd.buswidth >> 1));
+	rpc->totalxferlen = 1;
+	rpc->xferlen = 0;
+	rpc->addr = 0;
+
+	if (op->addr.nbytes) {
+		rpc->smenr |= RPC_SMENR_ADB(fls(op->addr.buswidth >> 1));
+		if (op->addr.nbytes == 4)
+			rpc->smenr |= RPC_SMENR_ADE(0xf);
+		else
+			rpc->smenr |= RPC_SMENR_ADE(0x7);
+
+		if (!offs && !len)
+			rpc->addr = *(u32 *)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)) {
+		rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer(op->data.nbytes)) |
+			      RPC_SMENR_SPIDB(fls(op->data.buswidth >> 1));
+
+		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->xferlen = *(u32 *)len;
+			rpc->totalxferlen += *(u32 *)len;
+		} else {
+			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)
+		return false;
+
+	if (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;
+
+	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);
+
+	writel(RPC_CMNCR_SFDE | RPC_CMNCR_MOIIO_HIZ |
+	       RPC_CMNCR_IOFV_HIZ | RPC_CMNCR_BSZ(0), rpc->regs + RPC_CMNCR);
+
+	writel(RPC_DRCR_RBURST(0x1f) | RPC_DRCR_RBE, rpc->regs + RPC_DRCR);
+	writel(rpc->cmd, rpc->regs + RPC_DRCMR);
+	writel(RPC_DREAR_EAC, rpc->regs + RPC_DREAR);
+	writel(0, rpc->regs + RPC_DROPR);
+	writel(rpc->smenr, rpc->regs + RPC_DRENR);
+	writel(rpc->dummy, rpc->regs + RPC_DRDMCR);
+	writel(0x0, rpc->regs + RPC_DRDRENR);
+	memcpy_fromio(buf, rpc->linear.map + 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 tx_offs, ret;
+
+	if (WARN_ON(offs + desc->info.offset + len > U32_MAX))
+		return -EINVAL;
+
+	if (WARN_ON(len > RPC_WBUF_SIZE))
+		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);
+
+	writel(RPC_CMNCR_MD | RPC_CMNCR_SFDE | RPC_CMNCR_MOIIO_HIZ |
+	       RPC_CMNCR_IOFV_HIZ | RPC_CMNCR_BSZ(0), rpc->regs + RPC_CMNCR);
+	writel(0x0, rpc->regs + RPC_SMDRENR);
+
+	writel(RPC_PHYCNT_CAL | 0x260 | RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF,
+	       rpc->regs + RPC_PHYCNT);
+
+	for (tx_offs = 0; tx_offs < RPC_WBUF_SIZE; tx_offs += 4)
+		writel(*(u32 *)(buf + tx_offs), rpc->regs + RPC_WBUF + tx_offs);
+
+	writel(rpc->cmd, rpc->regs + RPC_SMCMR);
+	writel(offs, rpc->regs + RPC_SMADR);
+	writel(rpc->smenr, rpc->regs + RPC_SMENR);
+	writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
+	ret = wait_msg_xfer_end(rpc);
+	if (ret)
+		goto out;
+
+	writel(RPC_DRCR_RCF, rpc->regs + RPC_DRCR);
+	writel(RPC_PHYCNT_CAL | RPC_PHYCNT_STRTIM(0) | 0x260,
+	       rpc->regs + RPC_PHYCNT);
+
+	return len;
+out:
+	return ret;
+}
+
+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->linear.map &&
+	    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;
+	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;
+		}
+	}
+
+	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(fls(xfer[0].tx_nbits >> 1));
+	rpc->addr = 0;
+
+	if (xfercnt > 2 && xfer[1].len && xfer[1].tx_buf) {
+		rpc->smenr |= RPC_SMENR_ADB(fls(xfer[1].tx_nbits >> 1));
+		for (i = 0; i < xfer[1].len; i++)
+			rpc->addr |= (u32)((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);
+	}
+
+	switch (xfercnt) {
+	case 2:
+		if (xfer[1].rx_buf) {
+			rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer
+				      (xfer[1].len)) | RPC_SMENR_SPIDB(fls
+				      (xfer[1].rx_nbits >> 1));
+			rpc->smcr = RPC_SMCR_SPIRE;
+			rpc->xfer_dir = SPI_MEM_DATA_IN;
+		} else if (xfer[1].tx_buf) {
+			rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer
+				      (xfer[1].len)) | RPC_SMENR_SPIDB(fls
+				      (xfer[1].tx_nbits >> 1));
+			rpc->smcr = RPC_SMCR_SPIWE;
+			rpc->xfer_dir = SPI_MEM_DATA_OUT;
+		}
+		break;
+
+	case 3:
+		if (xfer[2].len && xfer[2].rx_buf && !xfer[2].tx_buf) {
+			rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer
+				      (xfer[2].len)) | RPC_SMENR_SPIDB(fls
+				      (xfer[2].rx_nbits >> 1));
+			rpc->smcr = RPC_SMCR_SPIRE;
+			rpc->xfer_dir = SPI_MEM_DATA_IN;
+		} else if (xfer[2].len && xfer[2].tx_buf && !xfer[2].rx_buf) {
+			rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer
+				      (xfer[2].len)) | RPC_SMENR_SPIDB(fls
+				      (xfer[2].tx_nbits >> 1));
+			rpc->smcr = RPC_SMCR_SPIWE;
+			rpc->xfer_dir = SPI_MEM_DATA_OUT;
+		}
+
+		break;
+
+	case 4:
+		if (xfer[2].len && xfer[2].tx_buf) {
+			rpc->smenr |= RPC_SMENR_DME;
+			rpc->dummy = RPC_SMDMCR_DMCYC(xfer[2].len);
+			writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
+		}
+
+		if (xfer[3].len && xfer[3].rx_buf) {
+			rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer
+				      (xfer[3].len)) | RPC_SMENR_SPIDB(fls
+				      (xfer[3].rx_nbits >> 1));
+			rpc->smcr = RPC_SMCR_SPIRE;
+			rpc->xfer_dir = SPI_MEM_DATA_IN;
+		}
+
+		break;
+
+	default:
+		break;
+	}
+}
+
+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)) {
+			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 int __maybe_unused rpc_spi_runtime_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct spi_master *master = platform_get_drvdata(pdev);
+	struct rpc_spi *rpc = spi_master_get_devdata(master);
+
+	clk_disable_unprepare(rpc->clk_rpc);
+
+	return 0;
+}
+
+static int __maybe_unused rpc_spi_runtime_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct spi_master *master = platform_get_drvdata(pdev);
+	struct rpc_spi *rpc = spi_master_get_devdata(master);
+	int ret;
+
+	ret = clk_prepare_enable(rpc->clk_rpc);
+	if (ret)
+		dev_err(dev, "Can't enable rpc->clk_rpc\n");
+
+	return ret;
+}
+
+static const struct dev_pm_ops rpc_spi_dev_pm_ops = {
+	SET_RUNTIME_PM_OPS(rpc_spi_runtime_suspend,
+			   rpc_spi_runtime_resume, NULL)
+};
+
+static int rpc_spi_probe(struct platform_device *pdev)
+{
+	struct spi_master *master;
+	struct resource *res;
+	struct rpc_spi *rpc;
+	int ret;
+
+	master = spi_alloc_master(&pdev->dev, sizeof(struct rpc_spi));
+	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, "clk_rpc");
+	if (IS_ERR(rpc->clk_rpc))
+		return PTR_ERR(rpc->clk_rpc);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "rpc_regs");
+	rpc->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(rpc->regs))
+		return PTR_ERR(rpc->regs);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dirmap");
+	rpc->linear.map = devm_ioremap_resource(&pdev->dev, res);
+	if (!IS_ERR(rpc->linear.map)) {
+		rpc->linear.dma = res->start;
+		rpc->linear.size = resource_size(res);
+	} else {
+		rpc->linear.map = NULL;
+	}
+
+	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 |
+			SPI_RX_DUAL | SPI_TX_DUAL |
+			SPI_RX_QUAD | SPI_TX_QUAD;
+
+	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,rpc-r8a77995", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rpc_spi_of_ids);
+
+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 = &rpc_spi_dev_pm_ops,
+	},
+};
+module_platform_driver(rpc_spi_driver);
+
+MODULE_AUTHOR("Mason Yang <masonccyang@mxic.com.tw>");
+MODULE_DESCRIPTION("Renesas R-Car D3 RPC SPI controller driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

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

[Marek Vasut]

On 11/19/2018 11:01 AM, Mason Yang wrote:
> Add a driver for Renesas R-Car D3 RPC SPI controller driver.

The RPC supports both HF and SPI, not just SPI. And it's present in all
of Gen3 , not just D3 .

[...]

> +++ b/drivers/spi/spi-renesas-rpc.c
> @@ -0,0 +1,750 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (C) 2018 ~ 2019 Renesas Solutions Corp.
> +// Copyright (C) 2018 Macronix International Co., Ltd.
> +//
> +// R-Car D3 RPC SPI/QSPI/Octa driver
> +//
> +// Authors:
> +//	Mason Yang <masonccyang@mxic.com.tw>
> +//

Fix multiline comment please.

> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spi-mem.h>

[...]

> +#define RPC_CMNSR		0x0048	/* R */
> +#define RPC_CMNSR_SSLF		BIT(1)
> +#define	RPC_CMNSR_TEND		BIT(0)

#define[SPACE] instead of tab

> +#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_MEM(v)	(((v) & 0x3) << 0)
> +
> +#define RPC_PHYOFFSET1		0x0080	/* R/W */
> +#define RPC_PHYOFFSET2		0x0084	/* R/W */
> +
> +#define RPC_WBUF		0x8000	/* Write Buffer */
> +#define RPC_WBUF_SIZE		256	/* Write Buffer size */
> +
> +struct rpc_spi {
> +	struct clk *clk_rpc;
> +	void __iomem *regs;
> +	struct {
> +		void __iomem *map;
> +		dma_addr_t dma;
> +		size_t size;
> +	} linear;

Does this need it's own struct ?

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

This register cache might be a good candidate for regmap ?

> +	enum spi_mem_data_dir xfer_dir;
> +};
> +
> +static int rpc_spi_set_freq(struct rpc_spi *rpc, unsigned long freq)
> +{
> +	int ret;
> +
> +	if (rpc->cur_speed_hz == freq)
> +		return 0;
> +
> +	clk_disable_unprepare(rpc->clk_rpc);
> +	ret = clk_set_rate(rpc->clk_rpc, freq);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(rpc->clk_rpc);
> +	if (ret)
> +		return ret;

Is this clock disable/update/enable really needed ? I'd think that
clk_set_rate() would handle the rate update correctly.

> +	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.
> +	 */

FYI:
http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/spi/renesas_rpc_spi.c#l207

I think the STRTIM should be 6 .

> +	writel(RPC_PHYCNT_CAL | RPC_PHYCNT_STRTIM(0x3) | 0x260,
> +	       rpc->regs + RPC_PHYCNT);
> +
> +	/*
> +	 * NOTE: The 0x31511144 and 0x431 are undocumented bits,
> +	 *	 but they must be set for RPC_PHYOFFSET1 & RPC_PHYOFFSET2.
> +	 */
> +	writel(0x31511144, rpc->regs + RPC_PHYOFFSET1);
> +	writel(0x431, rpc->regs + RPC_PHYOFFSET2);
> +
> +	writel(RPC_SSLDR_SPNDL(7) | RPC_SSLDR_SLNDL(7) |
> +	       RPC_SSLDR_SCKDL(7), rpc->regs + RPC_SSLDR);
> +}
> +
> +static int wait_msg_xfer_end(struct rpc_spi *rpc)
> +{
> +	u32 sts;
> +
> +	return readl_poll_timeout(rpc->regs + RPC_CMNSR, sts,
> +				  sts & RPC_CMNSR_TEND, 0, USEC_PER_SEC);
> +}
> +
> +static u8 rpc_bits_xfer(u32 nbytes)
> +{
> +	u8 databyte;
> +
> +	switch (nbytes) {

Did you ever test unaligned writes and reads ? There are some nasty edge
cases in those.

Also, I think you can calculate the number of set bits using a simple
function, so the switch-case might not even be needed.

> +	case 1:
> +		databyte = 0x8;
> +		break;
> +	case 2:
> +		databyte = 0xc;
> +		break;
> +	default:
> +		databyte = 0xf;
> +		break;
> +	}
> +
> +	return databyte;
> +}
> +
> +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;
> +
> +	writel(RPC_CMNCR_MD | RPC_CMNCR_SFDE | RPC_CMNCR_MOIIO_HIZ |
> +	       RPC_CMNCR_IOFV_HIZ | RPC_CMNCR_BSZ(0), rpc->regs + RPC_CMNCR);
> +	writel(0x0, rpc->regs + RPC_SMDRENR);
> +
> +	if (tx_buf) {
> +		writel(rpc->cmd, rpc->regs + RPC_SMCMR);
> +		writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
> +		writel(rpc->addr, rpc->regs + RPC_SMADR);
> +		smenr = rpc->smenr;
> +
> +		while (pos < rpc->xferlen) {
> +			u32 nbytes = rpc->xferlen  - pos;
> +
> +			writel(*(u32 *)(tx_buf + pos), rpc->regs + RPC_SMWDR0);
> +
> +			if (nbytes > 4) {
> +				nbytes = 4;
> +				smcr = rpc->smcr |
> +				       RPC_SMCR_SPIE | RPC_SMCR_SSLKP;
> +			} else {
> +				smcr = rpc->smcr | RPC_SMCR_SPIE;
> +			}
> +
> +			writel(smenr, rpc->regs + RPC_SMENR);
> +			writel(smcr, rpc->regs + RPC_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) {
> +		while (pos < rpc->xferlen) {
> +			u32 nbytes = rpc->xferlen  - pos;
> +
> +			if (nbytes > 4)
> +				nbytes = 4;
> +
> +			writel(rpc->cmd, rpc->regs + RPC_SMCMR);
> +			writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
> +			writel(rpc->addr + pos, rpc->regs + RPC_SMADR);
> +			writel(rpc->smenr, rpc->regs + RPC_SMENR);
> +			writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
> +			ret = wait_msg_xfer_end(rpc);
> +			if (ret)
> +				goto out;
> +
> +			data = readl(rpc->regs + RPC_SMRDR0);
> +			memcpy_fromio(rx_buf + pos, (void *)&data, nbytes);
> +			pos += nbytes;
> +		}
> +	} else {
> +		writel(rpc->cmd, rpc->regs + RPC_SMCMR);
> +		writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
> +		writel(rpc->addr + pos, rpc->regs + RPC_SMADR);
> +		writel(rpc->smenr, rpc->regs + RPC_SMENR);
> +		writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
> +		ret = wait_msg_xfer_end(rpc);
> +	}
> +out:

Dont you need to stop the RPC somehow in case the transmission fails ?

> +	return ret;
> +}
> +
> +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(fls(op->cmd.buswidth >> 1));
> +	rpc->totalxferlen = 1;
> +	rpc->xferlen = 0;
> +	rpc->addr = 0;
> +
> +	if (op->addr.nbytes) {
> +		rpc->smenr |= RPC_SMENR_ADB(fls(op->addr.buswidth >> 1));
> +		if (op->addr.nbytes == 4)
> +			rpc->smenr |= RPC_SMENR_ADE(0xf);
> +		else
> +			rpc->smenr |= RPC_SMENR_ADE(0x7);
> +
> +		if (!offs && !len)
> +			rpc->addr = *(u32 *)offs;

How does this work ? Shouldn't this be just *offs to dereference the
pointer ?

> +		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)) {
> +		rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer(op->data.nbytes)) |
> +			      RPC_SMENR_SPIDB(fls(op->data.buswidth >> 1));
> +
> +		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->xferlen = *(u32 *)len;
> +			rpc->totalxferlen += *(u32 *)len;
> +		} else {
> +			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)
> +		return false;
> +
> +	if (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;
> +
> +	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);
> +
> +	writel(RPC_CMNCR_SFDE | RPC_CMNCR_MOIIO_HIZ |
> +	       RPC_CMNCR_IOFV_HIZ | RPC_CMNCR_BSZ(0), rpc->regs + RPC_CMNCR);
> +
> +	writel(RPC_DRCR_RBURST(0x1f) | RPC_DRCR_RBE, rpc->regs + RPC_DRCR);
> +	writel(rpc->cmd, rpc->regs + RPC_DRCMR);
> +	writel(RPC_DREAR_EAC, rpc->regs + RPC_DREAR);
> +	writel(0, rpc->regs + RPC_DROPR);
> +	writel(rpc->smenr, rpc->regs + RPC_DRENR);
> +	writel(rpc->dummy, rpc->regs + RPC_DRDMCR);
> +	writel(0x0, rpc->regs + RPC_DRDRENR);
> +	memcpy_fromio(buf, rpc->linear.map + 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 tx_offs, ret;
> +
> +	if (WARN_ON(offs + desc->info.offset + len > U32_MAX))
> +		return -EINVAL;
> +
> +	if (WARN_ON(len > RPC_WBUF_SIZE))
> +		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);
> +
> +	writel(RPC_CMNCR_MD | RPC_CMNCR_SFDE | RPC_CMNCR_MOIIO_HIZ |
> +	       RPC_CMNCR_IOFV_HIZ | RPC_CMNCR_BSZ(0), rpc->regs + RPC_CMNCR);
> +	writel(0x0, rpc->regs + RPC_SMDRENR);
> +
> +	writel(RPC_PHYCNT_CAL | 0x260 | RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF,
> +	       rpc->regs + RPC_PHYCNT);
> +
> +	for (tx_offs = 0; tx_offs < RPC_WBUF_SIZE; tx_offs += 4)
> +		writel(*(u32 *)(buf + tx_offs), rpc->regs + RPC_WBUF + tx_offs);

Isn't this some memcpy_toio() or iowrite32_rep() reimplementation here ?

> +	writel(rpc->cmd, rpc->regs + RPC_SMCMR);
> +	writel(offs, rpc->regs + RPC_SMADR);
> +	writel(rpc->smenr, rpc->regs + RPC_SMENR);
> +	writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
> +	ret = wait_msg_xfer_end(rpc);
> +	if (ret)
> +		goto out;
> +
> +	writel(RPC_DRCR_RCF, rpc->regs + RPC_DRCR);
> +	writel(RPC_PHYCNT_CAL | RPC_PHYCNT_STRTIM(0) | 0x260,
> +	       rpc->regs + RPC_PHYCNT);
> +
> +	return len;
> +out:

Shouldn't you shut the controller down if the xfer fails ?

> +	return ret;
> +}
> +
> +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->linear.map &&
> +	    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;
> +	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;
> +		}
> +	}
> +
> +	xfercnt = xferpos;
> +	rpc->xferlen = xfer[--xferpos].len;
> +	rpc->cmd = RPC_SMCMR_CMD(((u8 *)xfer[0].tx_buf)[0]);

Is the cast needed ?

> +	rpc->smenr = RPC_SMENR_CDE | RPC_SMENR_CDB(fls(xfer[0].tx_nbits >> 1));
> +	rpc->addr = 0;
> +
> +	if (xfercnt > 2 && xfer[1].len && xfer[1].tx_buf) {
> +		rpc->smenr |= RPC_SMENR_ADB(fls(xfer[1].tx_nbits >> 1));
> +		for (i = 0; i < xfer[1].len; i++)
> +			rpc->addr |= (u32)((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);
> +	}
> +
> +	switch (xfercnt) {
> +	case 2:
> +		if (xfer[1].rx_buf) {
> +			rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer
> +				      (xfer[1].len)) | RPC_SMENR_SPIDB(fls
> +				      (xfer[1].rx_nbits >> 1));

How much of this register value calculation could be somehow
deduplicated ? It seems to be almost the same thing copied thrice here.

> +			rpc->smcr = RPC_SMCR_SPIRE;
> +			rpc->xfer_dir = SPI_MEM_DATA_IN;
> +		} else if (xfer[1].tx_buf) {
> +			rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer
> +				      (xfer[1].len)) | RPC_SMENR_SPIDB(fls
> +				      (xfer[1].tx_nbits >> 1));
> +			rpc->smcr = RPC_SMCR_SPIWE;
> +			rpc->xfer_dir = SPI_MEM_DATA_OUT;
> +		}
> +		break;
> +
> +	case 3:
> +		if (xfer[2].len && xfer[2].rx_buf && !xfer[2].tx_buf) {
> +			rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer
> +				      (xfer[2].len)) | RPC_SMENR_SPIDB(fls
> +				      (xfer[2].rx_nbits >> 1));
> +			rpc->smcr = RPC_SMCR_SPIRE;
> +			rpc->xfer_dir = SPI_MEM_DATA_IN;
> +		} else if (xfer[2].len && xfer[2].tx_buf && !xfer[2].rx_buf) {
> +			rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer
> +				      (xfer[2].len)) | RPC_SMENR_SPIDB(fls
> +				      (xfer[2].tx_nbits >> 1));
> +			rpc->smcr = RPC_SMCR_SPIWE;
> +			rpc->xfer_dir = SPI_MEM_DATA_OUT;
> +		}
> +
> +		break;
> +
> +	case 4:
> +		if (xfer[2].len && xfer[2].tx_buf) {
> +			rpc->smenr |= RPC_SMENR_DME;
> +			rpc->dummy = RPC_SMDMCR_DMCYC(xfer[2].len);
> +			writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
> +		}
> +
> +		if (xfer[3].len && xfer[3].rx_buf) {
> +			rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer
> +				      (xfer[3].len)) | RPC_SMENR_SPIDB(fls
> +				      (xfer[3].rx_nbits >> 1));
> +			rpc->smcr = RPC_SMCR_SPIRE;
> +			rpc->xfer_dir = SPI_MEM_DATA_IN;
> +		}
> +
> +		break;
> +
> +	default:
> +		break;
> +	}
> +}
> +
> +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)) {

if (!list...)
 continue;

to reduce the indent level.

> +			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 int __maybe_unused rpc_spi_runtime_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct spi_master *master = platform_get_drvdata(pdev);
> +	struct rpc_spi *rpc = spi_master_get_devdata(master);
> +
> +	clk_disable_unprepare(rpc->clk_rpc);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused rpc_spi_runtime_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct spi_master *master = platform_get_drvdata(pdev);
> +	struct rpc_spi *rpc = spi_master_get_devdata(master);
> +	int ret;
> +
> +	ret = clk_prepare_enable(rpc->clk_rpc);
> +	if (ret)
> +		dev_err(dev, "Can't enable rpc->clk_rpc\n");
> +
> +	return ret;
> +}
> +
> +static const struct dev_pm_ops rpc_spi_dev_pm_ops = {
> +	SET_RUNTIME_PM_OPS(rpc_spi_runtime_suspend,
> +			   rpc_spi_runtime_resume, NULL)
> +};
> +
> +static int rpc_spi_probe(struct platform_device *pdev)
> +{
> +	struct spi_master *master;
> +	struct resource *res;
> +	struct rpc_spi *rpc;
> +	int ret;
> +
> +	master = spi_alloc_master(&pdev->dev, sizeof(struct rpc_spi));
> +	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, "clk_rpc");
> +	if (IS_ERR(rpc->clk_rpc))
> +		return PTR_ERR(rpc->clk_rpc);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "rpc_regs");
> +	rpc->regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(rpc->regs))
> +		return PTR_ERR(rpc->regs);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dirmap");
> +	rpc->linear.map = devm_ioremap_resource(&pdev->dev, res);
> +	if (!IS_ERR(rpc->linear.map)) {
> +		rpc->linear.dma = res->start;
> +		rpc->linear.size = resource_size(res);
> +	} else {
> +		rpc->linear.map = NULL;
> +	}
> +
> +	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 |
> +			SPI_RX_DUAL | SPI_TX_DUAL |
> +			SPI_RX_QUAD | SPI_TX_QUAD;
> +
> +	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,rpc-r8a77995", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, rpc_spi_of_ids);
> +
> +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 = &rpc_spi_dev_pm_ops,
> +	},
> +};
> +module_platform_driver(rpc_spi_driver);
> +
> +MODULE_AUTHOR("Mason Yang <masonccyang@mxic.com.tw>");
> +MODULE_DESCRIPTION("Renesas R-Car D3 RPC SPI controller driver");

This is not D3 specific and not SPI-only controller btw.

> +MODULE_LICENSE("GPL v2");
> 


-- 
Best regards,
Marek Vasut

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 606 bytes --]

On Mon, Nov 19, 2018 at 03:12:00PM +0100, Marek Vasut wrote:
> On 11/19/2018 11:01 AM, Mason Yang wrote:

> > +++ b/drivers/spi/spi-renesas-rpc.c
> > @@ -0,0 +1,750 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// Copyright (C) 2018 ~ 2019 Renesas Solutions Corp.
> > +// Copyright (C) 2018 Macronix International Co., Ltd.
> > +//
> > +// R-Car D3 RPC SPI/QSPI/Octa driver
> > +//
> > +// Authors:
> > +//	Mason Yang <masonccyang@mxic.com.tw>
> > +//

> Fix multiline comment please.

The SPDX header needs to be C++ style so I push people to make the whole
block C++ otherwise it looks messy.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Marek Vasut]

On 11/19/2018 04:27 PM, Mark Brown wrote:
> On Mon, Nov 19, 2018 at 03:12:00PM +0100, Marek Vasut wrote:
>> On 11/19/2018 11:01 AM, Mason Yang wrote:
> 
>>> +++ b/drivers/spi/spi-renesas-rpc.c
>>> @@ -0,0 +1,750 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +//
>>> +// Copyright (C) 2018 ~ 2019 Renesas Solutions Corp.
>>> +// Copyright (C) 2018 Macronix International Co., Ltd.
>>> +//
>>> +// R-Car D3 RPC SPI/QSPI/Octa driver
>>> +//
>>> +// Authors:
>>> +//	Mason Yang <masonccyang@mxic.com.tw>
>>> +//
> 
>> Fix multiline comment please.
> 
> The SPDX header needs to be C++ style so I push people to make the whole
> block C++ otherwise it looks messy.

OK, I'm not gonna wrestle you on this, but I think it looks horrible ;-)

-- 
Best regards,
Marek Vasut

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 358 bytes --]

On Mon, Nov 19, 2018 at 11:10:04PM +0100, Marek Vasut wrote:
> On 11/19/2018 04:27 PM, Mark Brown wrote:

> > The SPDX header needs to be C++ style so I push people to make the whole
> > block C++ otherwise it looks messy.

> OK, I'm not gonna wrestle you on this, but I think it looks horrible ;-)

I don't really like the C++ comment in the first place :(

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Marek Vasut]

On 11/20/2018 02:26 PM, Mark Brown wrote:
> On Mon, Nov 19, 2018 at 11:10:04PM +0100, Marek Vasut wrote:
>> On 11/19/2018 04:27 PM, Mark Brown wrote:
> 
>>> The SPDX header needs to be C++ style so I push people to make the whole
>>> block C++ otherwise it looks messy.
> 
>> OK, I'm not gonna wrestle you on this, but I think it looks horrible ;-)
> 
> I don't really like the C++ comment in the first place :(

Then we are in agreement :-)

-- 
Best regards,
Marek Vasut

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

[masonccyang]

[-- Attachment #1: Type: text/plain, Size: 12597 bytes --]

Hi Marek,


> Marek Vasut <marek.vasut@gmail.com> 
> 2018/11/19 下午 10:12
> 
> To
> 
> > +
> > +static int rpc_spi_set_freq(struct rpc_spi *rpc, unsigned long freq)
> > +{
> > +   int ret;
> > +
> > +   if (rpc->cur_speed_hz == freq)
> > +      return 0;
> > +
> > +   clk_disable_unprepare(rpc->clk_rpc);
> > +   ret = clk_set_rate(rpc->clk_rpc, freq);
> > +   if (ret)
> > +      return ret;
> > +
> > +   ret = clk_prepare_enable(rpc->clk_rpc);
> > +   if (ret)
> > +      return ret;
> 
> Is this clock disable/update/enable really needed ? I'd think that
> clk_set_rate() would handle the rate update correctly.

This is for run time PM mechanism in spi-mem layer and __spi_sync(),
you may refer to another patch [1].

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git/commit/?h=for-4.21&id=b942d80b0a394e8ea18fce3b032b4700439e8ca3


> 
> > +   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.
> > +    */
> 
> FYI:
> 
http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/spi/renesas_rpc_spi.c#l207

> 
> I think the STRTIM should be 6 .
> 

In my D3 Draak board, the STRTIM is 0x3 for on board qspi flash and 
mx25uw51245g.
And this is also refer to Renesas R-Car Gen3 bare-metal code, mini-monitor 
v4.01.


> > +   writel(RPC_PHYCNT_CAL | RPC_PHYCNT_STRTIM(0x3) | 0x260,
> > +          rpc->regs + RPC_PHYCNT);
> > +
> > +   /*
> > +    * NOTE: The 0x31511144 and 0x431 are undocumented bits,
> > +    *    but they must be set for RPC_PHYOFFSET1 & RPC_PHYOFFSET2.
> > +    */
> > +   writel(0x31511144, rpc->regs + RPC_PHYOFFSET1);
> > +   writel(0x431, rpc->regs + RPC_PHYOFFSET2);
> > +
> > +   writel(RPC_SSLDR_SPNDL(7) | RPC_SSLDR_SLNDL(7) |
> > +          RPC_SSLDR_SCKDL(7), rpc->regs + RPC_SSLDR);
> > +}
> > +
> > +static int wait_msg_xfer_end(struct rpc_spi *rpc)
> > +{
> > +   u32 sts;
> > +
> > +   return readl_poll_timeout(rpc->regs + RPC_CMNSR, sts,
> > +              sts & RPC_CMNSR_TEND, 0, USEC_PER_SEC);
> > +}
> > +
> > +static u8 rpc_bits_xfer(u32 nbytes)
> > +{
> > +   u8 databyte;
> > +
> > +   switch (nbytes) {
> 
> Did you ever test unaligned writes and reads ? There are some nasty edge
> cases in those.
> 
> Also, I think you can calculate the number of set bits using a simple
> function, so the switch-case might not even be needed.
> 

okay.


> > +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;
> > +
> > +   writel(RPC_CMNCR_MD | RPC_CMNCR_SFDE | RPC_CMNCR_MOIIO_HIZ |
> > +          RPC_CMNCR_IOFV_HIZ | RPC_CMNCR_BSZ(0), rpc->regs + 
RPC_CMNCR);
> > +   writel(0x0, rpc->regs + RPC_SMDRENR);
> > +
> > +   if (tx_buf) {
> > +      writel(rpc->cmd, rpc->regs + RPC_SMCMR);
> > +      writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
> > +      writel(rpc->addr, rpc->regs + RPC_SMADR);
> > +      smenr = rpc->smenr;
> > +
> > +      while (pos < rpc->xferlen) {
> > +         u32 nbytes = rpc->xferlen  - pos;
> > +
> > +         writel(*(u32 *)(tx_buf + pos), rpc->regs + RPC_SMWDR0);
> > +
> > +         if (nbytes > 4) {
> > +            nbytes = 4;
> > +            smcr = rpc->smcr |
> > +                   RPC_SMCR_SPIE | RPC_SMCR_SSLKP;
> > +         } else {
> > +            smcr = rpc->smcr | RPC_SMCR_SPIE;
> > +         }
> > +
> > +         writel(smenr, rpc->regs + RPC_SMENR);
> > +         writel(smcr, rpc->regs + RPC_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) {
> > +      while (pos < rpc->xferlen) {
> > +         u32 nbytes = rpc->xferlen  - pos;
> > +
> > +         if (nbytes > 4)
> > +            nbytes = 4;
> > +
> > +         writel(rpc->cmd, rpc->regs + RPC_SMCMR);
> > +         writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
> > +         writel(rpc->addr + pos, rpc->regs + RPC_SMADR);
> > +         writel(rpc->smenr, rpc->regs + RPC_SMENR);
> > +         writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
> > +         ret = wait_msg_xfer_end(rpc);
> > +         if (ret)
> > +            goto out;
> > +
> > +         data = readl(rpc->regs + RPC_SMRDR0);
> > +         memcpy_fromio(rx_buf + pos, (void *)&data, nbytes);
> > +         pos += nbytes;
> > +      }
> > +   } else {
> > +      writel(rpc->cmd, rpc->regs + RPC_SMCMR);
> > +      writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
> > +      writel(rpc->addr + pos, rpc->regs + RPC_SMADR);
> > +      writel(rpc->smenr, rpc->regs + RPC_SMENR);
> > +      writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
> > +      ret = wait_msg_xfer_end(rpc);
> > +   }
> > +out:
> 
> Dont you need to stop the RPC somehow in case the transmission fails ?

It seems there is no any RPC registers bit to monitor xfer fail !


> 
> > +   return ret;
> > +}
> > +
> > +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(fls(op->cmd.buswidth >> 1));
> > +   rpc->totalxferlen = 1;
> > +   rpc->xferlen = 0;
> > +   rpc->addr = 0;
> > +
> > +   if (op->addr.nbytes) {
> > +      rpc->smenr |= RPC_SMENR_ADB(fls(op->addr.buswidth >> 1));
> > +      if (op->addr.nbytes == 4)
> > +         rpc->smenr |= RPC_SMENR_ADE(0xf);
> > +      else
> > +         rpc->smenr |= RPC_SMENR_ADE(0x7);
> > +
> > +      if (!offs && !len)
> > +         rpc->addr = *(u32 *)offs;
> 
> How does this work ? Shouldn't this be just *offs to dereference the
> pointer ?

Oops, it's a mistake. I will patch it later. thanks.


> > +
> > +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 tx_offs, ret;
> > +
> > +   if (WARN_ON(offs + desc->info.offset + len > U32_MAX))
> > +      return -EINVAL;
> > +
> > +   if (WARN_ON(len > RPC_WBUF_SIZE))
> > +      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);
> > +
> > +   writel(RPC_CMNCR_MD | RPC_CMNCR_SFDE | RPC_CMNCR_MOIIO_HIZ |
> > +          RPC_CMNCR_IOFV_HIZ | RPC_CMNCR_BSZ(0), rpc->regs + 
RPC_CMNCR);
> > +   writel(0x0, rpc->regs + RPC_SMDRENR);
> > +
> > +   writel(RPC_PHYCNT_CAL | 0x260 | RPC_PHYCNT_WBUF2 | 
RPC_PHYCNT_WBUF,
> > +          rpc->regs + RPC_PHYCNT);
> > +
> > +   for (tx_offs = 0; tx_offs < RPC_WBUF_SIZE; tx_offs += 4)
> > +      writel(*(u32 *)(buf + tx_offs), rpc->regs + RPC_WBUF + 
tx_offs);
> 
> Isn't this some memcpy_toio() or iowrite32_rep() reimplementation here ?

I will check iowrite32_reg().


> > +static int rpc_spi_probe(struct platform_device *pdev)
> > +{
> > +   struct spi_master *master;
> > +   struct resource *res;
> > +   struct rpc_spi *rpc;
> > +   int ret;
> > +
> > +   master = spi_alloc_master(&pdev->dev, sizeof(struct rpc_spi));
> > +   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, "clk_rpc");
> > +   if (IS_ERR(rpc->clk_rpc))
> > +      return PTR_ERR(rpc->clk_rpc);
> > +
> > +   res = platform_get_resource_byname(pdev, IORESOURCE_MEM, 
"rpc_regs");
> > +   rpc->regs = devm_ioremap_resource(&pdev->dev, res);
> > +   if (IS_ERR(rpc->regs))
> > +      return PTR_ERR(rpc->regs);
> > +
> > +   res = platform_get_resource_byname(pdev, IORESOURCE_MEM, 
"dirmap");
> > +   rpc->linear.map = devm_ioremap_resource(&pdev->dev, res);
> > +   if (!IS_ERR(rpc->linear.map)) {
> > +      rpc->linear.dma = res->start;
> > +      rpc->linear.size = resource_size(res);
> > +   } else {
> > +      rpc->linear.map = NULL;
> > +   }
> > +
> > +   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 |
> > +         SPI_RX_DUAL | SPI_TX_DUAL |
> > +         SPI_RX_QUAD | SPI_TX_QUAD;
> > +
> > +   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,rpc-r8a77995", },
> > +   { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, rpc_spi_of_ids);
> > +
> > +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 = &rpc_spi_dev_pm_ops,
> > +   },
> > +};
> > +module_platform_driver(rpc_spi_driver);
> > +
> > +MODULE_AUTHOR("Mason Yang <masonccyang@mxic.com.tw>");
> > +MODULE_DESCRIPTION("Renesas R-Car D3 RPC SPI controller driver");
> 
> This is not D3 specific and not SPI-only controller btw.

In R-Car Gen3, there are some registers(i.e,. RPC_PHYCNT) in different 
setting 
for R-Car H3, M3-W, V3M, V3H, D3, M3-N and E3 model. 

I test this patch is based on D3 Draak board, it works fine but I am not 
sure 
if these registers setting is ok for others R-Card model.

I think this could be a reference when patch others Gen3 model is needed. 

Thanks for your comments & review.

best regards,
Mason





CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

[-- Attachment #2: Type: text/html, Size: 18868 bytes --]

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

[Marek Vasut]

On 11/20/2018 08:23 AM, masonccyang@mxic.com.tw wrote:
> Hi Marek,

Hi,

>> Marek Vasut <marek.vasut@gmail.com>
>> 2018/11/19 下午 10:12
>>
>> To
>>
>> > +
>> > +static int rpc_spi_set_freq(struct rpc_spi *rpc, unsigned long freq)
>> > +{
>> > +   int ret;
>> > +
>> > +   if (rpc->cur_speed_hz == freq)
>> > +      return 0;
>> > +
>> > +   clk_disable_unprepare(rpc->clk_rpc);
>> > +   ret = clk_set_rate(rpc->clk_rpc, freq);
>> > +   if (ret)
>> > +      return ret;
>> > +
>> > +   ret = clk_prepare_enable(rpc->clk_rpc);
>> > +   if (ret)
>> > +      return ret;
>>
>> Is this clock disable/update/enable really needed ? I'd think that
>> clk_set_rate() would handle the rate update correctly.
> 
> This is for run time PM mechanism in spi-mem layer and __spi_sync(),
> you may refer to another patch [1].
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git/commit/?h=for-4.21&id=b942d80b0a394e8ea18fce3b032b4700439e8ca3

I think Geert commented on the clock topic, so let's move it there.
Disabling and enabling clock to change their rate looks real odd to me.

>> > +   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.
>> > +    */
>>
>> FYI:
>>
> http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/spi/renesas_rpc_spi.c#l207
>>
>> I think the STRTIM should be 6 .
>>
> 
> In my D3 Draak board, the STRTIM is 0x3 for on board qspi flash and
> mx25uw51245g.
> And this is also refer to Renesas R-Car Gen3 bare-metal code,
> mini-monitor v4.01.

The copy of minimon I have says 6 , but maybe this is flash specific ?

[...]

>> > +         writel(rpc->cmd, rpc->regs + RPC_SMCMR);
>> > +         writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
>> > +         writel(rpc->addr + pos, rpc->regs + RPC_SMADR);
>> > +         writel(rpc->smenr, rpc->regs + RPC_SMENR);
>> > +         writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
>> > +         ret = wait_msg_xfer_end(rpc);
>> > +         if (ret)
>> > +            goto out;
>> > +
>> > +         data = readl(rpc->regs + RPC_SMRDR0);
>> > +         memcpy_fromio(rx_buf + pos, (void *)&data, nbytes);
>> > +         pos += nbytes;
>> > +      }
>> > +   } else {
>> > +      writel(rpc->cmd, rpc->regs + RPC_SMCMR);
>> > +      writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
>> > +      writel(rpc->addr + pos, rpc->regs + RPC_SMADR);
>> > +      writel(rpc->smenr, rpc->regs + RPC_SMENR);
>> > +      writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
>> > +      ret = wait_msg_xfer_end(rpc);
>> > +   }
>> > +out:
>>
>> Dont you need to stop the RPC somehow in case the transmission fails ?
> 
> It seems there is no any RPC registers bit to monitor xfer fail !

What happens if wait_msg_xfer_end() returns non-zero ? I guess that
means the transfer timed out ?

[...]

>> > +static const struct of_device_id rpc_spi_of_ids[] = {
>> > +   { .compatible = "renesas,rpc-r8a77995", },
>> > +   { /* sentinel */ }
>> > +};
>> > +MODULE_DEVICE_TABLE(of, rpc_spi_of_ids);
>> > +
>> > +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 = &rpc_spi_dev_pm_ops,
>> > +   },
>> > +};
>> > +module_platform_driver(rpc_spi_driver);
>> > +
>> > +MODULE_AUTHOR("Mason Yang <masonccyang@mxic.com.tw>");
>> > +MODULE_DESCRIPTION("Renesas R-Car D3 RPC SPI controller driver");
>>
>> This is not D3 specific and not SPI-only controller btw.
> 
> In R-Car Gen3, there are some registers(i.e,. RPC_PHYCNT) in different
> setting
> for R-Car H3, M3-W, V3M, V3H, D3, M3-N and E3 model.
> 
> I test this patch is based on D3 Draak board, it works fine but I am not
> sure
> if these registers setting is ok for others R-Card model.
> 
> I think this could be a reference when patch others Gen3 model is needed.
You can take a look into the U-Boot driver(s) I linked, that's used on
the other SoCs you listed (except for V3H).

-- 
Best regards,
Marek Vasut

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

[Boris Brezillon]

On Tue, 20 Nov 2018 14:09:05 +0100
Marek Vasut <marek.vasut@gmail.com> wrote:

> On 11/20/2018 08:23 AM, masonccyang@mxic.com.tw wrote:
> > Hi Marek,  
> 
> Hi,
> 
> >> Marek Vasut <marek.vasut@gmail.com>
> >> 2018/11/19 下午 10:12
> >>
> >> To
> >>  
> >> > +
> >> > +static int rpc_spi_set_freq(struct rpc_spi *rpc, unsigned long freq)
> >> > +{
> >> > +   int ret;
> >> > +
> >> > +   if (rpc->cur_speed_hz == freq)
> >> > +      return 0;
> >> > +
> >> > +   clk_disable_unprepare(rpc->clk_rpc);
> >> > +   ret = clk_set_rate(rpc->clk_rpc, freq);
> >> > +   if (ret)
> >> > +      return ret;
> >> > +
> >> > +   ret = clk_prepare_enable(rpc->clk_rpc);
> >> > +   if (ret)
> >> > +      return ret;  
> >>
> >> Is this clock disable/update/enable really needed ? I'd think that
> >> clk_set_rate() would handle the rate update correctly.  
> > 
> > This is for run time PM mechanism in spi-mem layer and __spi_sync(),
> > you may refer to another patch [1].
> > 
> > [1]
> > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git/commit/?h=for-4.21&id=b942d80b0a394e8ea18fce3b032b4700439e8ca3  
> 
> I think Geert commented on the clock topic, so let's move it there.
> Disabling and enabling clock to change their rate looks real odd to me.

Look at the CLK_SET_RATE_GATE definition and its users and you'll see
it's not unusual to have such constraints on clks. Maybe your HW does
not have such constraints, but it's not particularly odd to do that
(though it could probably be automated by the clk framework somehow).

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

[Marek Vasut]

On 11/20/2018 02:32 PM, Boris Brezillon wrote:
> On Tue, 20 Nov 2018 14:09:05 +0100
> Marek Vasut <marek.vasut@gmail.com> wrote:
> 
>> On 11/20/2018 08:23 AM, masonccyang@mxic.com.tw wrote:
>>> Hi Marek,  
>>
>> Hi,
>>
>>>> Marek Vasut <marek.vasut@gmail.com>
>>>> 2018/11/19 下午 10:12
>>>>
>>>> To
>>>>  
>>>>> +
>>>>> +static int rpc_spi_set_freq(struct rpc_spi *rpc, unsigned long freq)
>>>>> +{
>>>>> +   int ret;
>>>>> +
>>>>> +   if (rpc->cur_speed_hz == freq)
>>>>> +      return 0;
>>>>> +
>>>>> +   clk_disable_unprepare(rpc->clk_rpc);
>>>>> +   ret = clk_set_rate(rpc->clk_rpc, freq);
>>>>> +   if (ret)
>>>>> +      return ret;
>>>>> +
>>>>> +   ret = clk_prepare_enable(rpc->clk_rpc);
>>>>> +   if (ret)
>>>>> +      return ret;  
>>>>
>>>> Is this clock disable/update/enable really needed ? I'd think that
>>>> clk_set_rate() would handle the rate update correctly.  
>>>
>>> This is for run time PM mechanism in spi-mem layer and __spi_sync(),
>>> you may refer to another patch [1].
>>>
>>> [1]
>>> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git/commit/?h=for-4.21&id=b942d80b0a394e8ea18fce3b032b4700439e8ca3  
>>
>> I think Geert commented on the clock topic, so let's move it there.
>> Disabling and enabling clock to change their rate looks real odd to me.
> 
> Look at the CLK_SET_RATE_GATE definition and its users and you'll see
> it's not unusual to have such constraints on clks. Maybe your HW does
> not have such constraints, but it's not particularly odd to do that
> (though it could probably be automated by the clk framework somehow).

I think you stated my concern right at the end, good, no need for me to
add to this. Yes, I don't think any random driver should deal with
peculiarities of the clock controller.

-- 
Best regards,
Marek Vasut

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

[masonccyang]

[-- Attachment #1: Type: text/plain, Size: 10575 bytes --]


Hi Marek, 


> > +
> > +struct rpc_spi {
> > +   struct clk *clk_rpc;
> > +   void __iomem *regs;
> > +   struct {
> > +      void __iomem *map;
> > +      dma_addr_t dma;
> > +      size_t size;
> > +   } linear;
> 
> Does this need it's own struct ?
> 

yup, I think it's better.
In case no "dirmap" in dtb and no direct mapping mode implemented.


> > +   u32 cur_speed_hz;
> > +   u32 cmd;
> > +   u32 addr;
> > +   u32 dummy;
> > +   u32 smcr;
> > +   u32 smenr;
> > +   u32 xferlen;
> > +   u32 totalxferlen;
> 
> This register cache might be a good candidate for regmap ?

I don't know what does it mean ?
Could you give me more information!

> 
> > +   enum spi_mem_data_dir xfer_dir;
> > +};
> > +
> > +static int rpc_spi_set_freq(struct rpc_spi *rpc, unsigned long freq)
> > +{
> > +   int ret;
> > +
> > +   if (rpc->cur_speed_hz == freq)
> > +      return 0;
> > +
> > +   clk_disable_unprepare(rpc->clk_rpc);
> > +   ret = clk_set_rate(rpc->clk_rpc, freq);
> > +   if (ret)
> > +      return ret;
> > +
> > +   ret = clk_prepare_enable(rpc->clk_rpc);
> > +   if (ret)
> > +      return ret;
> 
> Is this clock disable/update/enable really needed ? I'd think that
> clk_set_rate() would handle the rate update correctly.

As Gerrt mentioned, I will remove them.


> > +static int wait_msg_xfer_end(struct rpc_spi *rpc)
> > +{
> > +   u32 sts;
> > +
> > +   return readl_poll_timeout(rpc->regs + RPC_CMNSR, sts,
> > +              sts & RPC_CMNSR_TEND, 0, USEC_PER_SEC);
> > +}
> > +
> > +static u8 rpc_bits_xfer(u32 nbytes)
> > +{
> > +   u8 databyte;
> > +
> > +   switch (nbytes) {
> 
> Did you ever test unaligned writes and reads ? There are some nasty edge
> cases in those.
> 
> Also, I think you can calculate the number of set bits using a simple
> function, so the switch-case might not even be needed.
> 

Any example function ?

> > +   case 1:
> > +      databyte = 0x8;
> > +      break;
> > +   case 2:
> > +      databyte = 0xc;
> > +      break;
> > +   default:
> > +      databyte = 0xf;
> > +      break;
> > +   }
> > +
> > +   return databyte;
> > +}
> > +
> > +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;
> > +
> > +   writel(RPC_CMNCR_MD | RPC_CMNCR_SFDE | RPC_CMNCR_MOIIO_HIZ |
> > +          RPC_CMNCR_IOFV_HIZ | RPC_CMNCR_BSZ(0), rpc->regs + 
RPC_CMNCR);
> > +   writel(0x0, rpc->regs + RPC_SMDRENR);
> > +
> > +   if (tx_buf) {
> > +      writel(rpc->cmd, rpc->regs + RPC_SMCMR);
> > +      writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
> > +      writel(rpc->addr, rpc->regs + RPC_SMADR);
> > +      smenr = rpc->smenr;
> > +
> > +      while (pos < rpc->xferlen) {
> > +         u32 nbytes = rpc->xferlen  - pos;
> > +
> > +         writel(*(u32 *)(tx_buf + pos), rpc->regs + RPC_SMWDR0);
> > +
> > +         if (nbytes > 4) {
> > +            nbytes = 4;
> > +            smcr = rpc->smcr |
> > +                   RPC_SMCR_SPIE | RPC_SMCR_SSLKP;
> > +         } else {
> > +            smcr = rpc->smcr | RPC_SMCR_SPIE;
> > +         }
> > +
> > +         writel(smenr, rpc->regs + RPC_SMENR);
> > +         writel(smcr, rpc->regs + RPC_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) {
> > +      while (pos < rpc->xferlen) {
> > +         u32 nbytes = rpc->xferlen  - pos;
> > +
> > +         if (nbytes > 4)
> > +            nbytes = 4;
> > +
> > +         writel(rpc->cmd, rpc->regs + RPC_SMCMR);
> > +         writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
> > +         writel(rpc->addr + pos, rpc->regs + RPC_SMADR);
> > +         writel(rpc->smenr, rpc->regs + RPC_SMENR);
> > +         writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
> > +         ret = wait_msg_xfer_end(rpc);
> > +         if (ret)
> > +            goto out;
> > +
> > +         data = readl(rpc->regs + RPC_SMRDR0);
> > +         memcpy_fromio(rx_buf + pos, (void *)&data, nbytes);
> > +         pos += nbytes;
> > +      }
> > +   } else {
> > +      writel(rpc->cmd, rpc->regs + RPC_SMCMR);
> > +      writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
> > +      writel(rpc->addr + pos, rpc->regs + RPC_SMADR);
> > +      writel(rpc->smenr, rpc->regs + RPC_SMENR);
> > +      writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
> > +      ret = wait_msg_xfer_end(rpc);
> > +   }
> > +out:
> 
> Dont you need to stop the RPC somehow in case the transmission fails ?
> 

I can't find any RPC registers can do this !

Do you know how to do this ?



> > +   writel(rpc->cmd, rpc->regs + RPC_SMCMR);
> > +   writel(offs, rpc->regs + RPC_SMADR);
> > +   writel(rpc->smenr, rpc->regs + RPC_SMENR);
> > +   writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
> > +   ret = wait_msg_xfer_end(rpc);
> > +   if (ret)
> > +      goto out;
> > +
> > +   writel(RPC_DRCR_RCF, rpc->regs + RPC_DRCR);
> > +   writel(RPC_PHYCNT_CAL | RPC_PHYCNT_STRTIM(0) | 0x260,
> > +          rpc->regs + RPC_PHYCNT);
> > +
> > +   return len;
> > +out:
> 
> Shouldn't you shut the controller down if the xfer fails ?

Any registers can shut down RPC controller ? 
SW reset ? 


> 
> > +   return ret;
> > +}
> > +
> > +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->linear.map &&
> > +       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;
> > +   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;
> > +      }
> > +   }
> > +
> > +   xfercnt = xferpos;
> > +   rpc->xferlen = xfer[--xferpos].len;
> > +   rpc->cmd = RPC_SMCMR_CMD(((u8 *)xfer[0].tx_buf)[0]);
> 
> Is the cast needed ?

? 

> 
> > +   rpc->smenr = RPC_SMENR_CDE | RPC_SMENR_CDB(fls(xfer[0].tx_nbits >> 
1));
> > +   rpc->addr = 0;
> > +
> > +   if (xfercnt > 2 && xfer[1].len && xfer[1].tx_buf) {
> > +      rpc->smenr |= RPC_SMENR_ADB(fls(xfer[1].tx_nbits >> 1));
> > +      for (i = 0; i < xfer[1].len; i++)
> > +         rpc->addr |= (u32)((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);
> > +   }
> > +
> > +   switch (xfercnt) {
> > +   case 2:
> > +      if (xfer[1].rx_buf) {
> > +         rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer
> > +                  (xfer[1].len)) | RPC_SMENR_SPIDB(fls
> > +                  (xfer[1].rx_nbits >> 1));
> 
> How much of this register value calculation could be somehow
> deduplicated ? It seems to be almost the same thing copied thrice here.

I don't get your point!

The 2'nd transfer may be 
1) spi-address 
2) tx_buf[] for write registers.
3) rx_buf[] for read status.

parse them and write to rpc->addr and so on.
Or you have a better way to do this ?


thanks again for your review & help.

best regards,
Mason


CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

[-- Attachment #2: Type: text/html, Size: 16272 bytes --]

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

[Marek Vasut]

On 11/23/2018 01:45 AM, masonccyang@mxic.com.tw wrote:
> Hi Marek,

Hi,

>> > +
>> > +struct rpc_spi {
>> > + � struct clk *clk_rpc;
>> > + � void __iomem *regs;
>> > + � struct {
>> > + � � �void __iomem *map;
>> > + � � �dma_addr_t dma;
>> > + � � �size_t size;
>> > + � } linear;
>>
>> Does this need it's own struct ?
>>
> 
> yup, I think it's better.
> In case no "dirmap" in dtb and no direct mapping mode implemented.
> 
> 
>> > + � u32 cur_speed_hz;
>> > + � u32 cmd;
>> > + � u32 addr;
>> > + � u32 dummy;
>> > + � u32 smcr;
>> > + � u32 smenr;
>> > + � u32 xferlen;
>> > + � u32 totalxferlen;
>>
>> This register cache might be a good candidate for regmap ?
> 
> I don't know what does it mean ?
> Could you give me more information!

See include/linux/regmap.h and git grep regmap drivers/ for examples.

>> > + � enum spi_mem_data_dir xfer_dir;
>> > +};
>> > +
>> > +static int rpc_spi_set_freq(struct rpc_spi *rpc, unsigned long freq)
>> > +{
>> > + � int ret;
>> > +
>> > + � if (rpc->cur_speed_hz == freq)
>> > + � � �return 0;
>> > +
>> > + � clk_disable_unprepare(rpc->clk_rpc);
>> > + � ret = clk_set_rate(rpc->clk_rpc, freq);
>> > + � if (ret)
>> > + � � �return ret;
>> > +
>> > + � ret = clk_prepare_enable(rpc->clk_rpc);
>> > + � if (ret)
>> > + � � �return ret;
>>
>> Is this clock disable/update/enable really needed ? I'd think that
>> clk_set_rate() would handle the rate update correctly.
> 
> As Gerrt mentioned, I will remove them.
> 
> 
>> > +static int wait_msg_xfer_end(struct rpc_spi *rpc)
>> > +{
>> > + � u32 sts;
>> > +
>> > + � return readl_poll_timeout(rpc->regs + RPC_CMNSR, sts,
>> > + � � � � � � �sts & RPC_CMNSR_TEND, 0, USEC_PER_SEC);
>> > +}
>> > +
>> > +static u8 rpc_bits_xfer(u32 nbytes)
>> > +{
>> > + � u8 databyte;
>> > +
>> > + � switch (nbytes) {
>>
>> Did you ever test unaligned writes and reads ? There are some nasty edge
>> cases in those.
>>
>> Also, I think you can calculate the number of set bits using a simple
>> function, so the switch-case might not even be needed.
>>
> 
> Any example function ?

Nope, you'd have to think of one. You need to fill $nbytes bits from top
down. I think you can somehow use GENMASK() .

>> > + � case 1:
>> > + � � �databyte = 0x8;
>> > + � � �break;
>> > + � case 2:
>> > + � � �databyte = 0xc;
>> > + � � �break;
>> > + � default:
>> > + � � �databyte = 0xf;
>> > + � � �break;
>> > + � }
>> > +
>> > + � return databyte;
>> > +}
>> > +
>> > +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;
>> > +
>> > + � writel(RPC_CMNCR_MD | RPC_CMNCR_SFDE | RPC_CMNCR_MOIIO_HIZ |
>> > + � � � � �RPC_CMNCR_IOFV_HIZ | RPC_CMNCR_BSZ(0), rpc->regs +
> RPC_CMNCR);
>> > + � writel(0x0, rpc->regs + RPC_SMDRENR);
>> > +
>> > + � if (tx_buf) {
>> > + � � �writel(rpc->cmd, rpc->regs + RPC_SMCMR);
>> > + � � �writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
>> > + � � �writel(rpc->addr, rpc->regs + RPC_SMADR);
>> > + � � �smenr = rpc->smenr;
>> > +
>> > + � � �while (pos < rpc->xferlen) {
>> > + � � � � u32 nbytes = rpc->xferlen �- pos;
>> > +
>> > + � � � � writel(*(u32 *)(tx_buf + pos), rpc->regs + RPC_SMWDR0);
>> > +
>> > + � � � � if (nbytes > 4) {
>> > + � � � � � �nbytes = 4;
>> > + � � � � � �smcr = rpc->smcr |
>> > + � � � � � � � � � RPC_SMCR_SPIE | RPC_SMCR_SSLKP;
>> > + � � � � } else {
>> > + � � � � � �smcr = rpc->smcr | RPC_SMCR_SPIE;
>> > + � � � � }
>> > +
>> > + � � � � writel(smenr, rpc->regs + RPC_SMENR);
>> > + � � � � writel(smcr, rpc->regs + RPC_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) {
>> > + � � �while (pos < rpc->xferlen) {
>> > + � � � � u32 nbytes = rpc->xferlen �- pos;
>> > +
>> > + � � � � if (nbytes > 4)
>> > + � � � � � �nbytes = 4;
>> > +
>> > + � � � � writel(rpc->cmd, rpc->regs + RPC_SMCMR);
>> > + � � � � writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
>> > + � � � � writel(rpc->addr + pos, rpc->regs + RPC_SMADR);
>> > + � � � � writel(rpc->smenr, rpc->regs + RPC_SMENR);
>> > + � � � � writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
>> > + � � � � ret = wait_msg_xfer_end(rpc);
>> > + � � � � if (ret)
>> > + � � � � � �goto out;
>> > +
>> > + � � � � data = readl(rpc->regs + RPC_SMRDR0);
>> > + � � � � memcpy_fromio(rx_buf + pos, (void *)&data, nbytes);
>> > + � � � � pos += nbytes;
>> > + � � �}
>> > + � } else {
>> > + � � �writel(rpc->cmd, rpc->regs + RPC_SMCMR);
>> > + � � �writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
>> > + � � �writel(rpc->addr + pos, rpc->regs + RPC_SMADR);
>> > + � � �writel(rpc->smenr, rpc->regs + RPC_SMENR);
>> > + � � �writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
>> > + � � �ret = wait_msg_xfer_end(rpc);
>> > + � }
>> > +out:
>>
>> Dont you need to stop the RPC somehow in case the transmission fails ?
>>
> 
> I can't find any RPC registers can do this !
> 
> Do you know how to do this ?

It should be in the RPC datasheet ? It's likely going to involve SMCR,
possibly clear SPIE bit and maybe some more.

>> > + � writel(rpc->cmd, rpc->regs + RPC_SMCMR);
>> > + � writel(offs, rpc->regs + RPC_SMADR);
>> > + � writel(rpc->smenr, rpc->regs + RPC_SMENR);
>> > + � writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
>> > + � ret = wait_msg_xfer_end(rpc);
>> > + � if (ret)
>> > + � � �goto out;
>> > +
>> > + � writel(RPC_DRCR_RCF, rpc->regs + RPC_DRCR);
>> > + � writel(RPC_PHYCNT_CAL | RPC_PHYCNT_STRTIM(0) | 0x260,
>> > + � � � � �rpc->regs + RPC_PHYCNT);
>> > +
>> > + � return len;
>> > +out:
>>
>> Shouldn't you shut the controller down if the xfer fails ?
> 
> Any registers can shut down RPC controller ?
> SW reset ?

Possibly, can you research it ?

>> > + � return ret;
>> > +}
>> > +
>> > +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->linear.map &&
>> > + � � � 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;
>> > + � 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;
>> > + � � �}
>> > + � }
>> > +
>> > + � xfercnt = xferpos;
>> > + � rpc->xferlen = xfer[--xferpos].len;
>> > + � rpc->cmd = RPC_SMCMR_CMD(((u8 *)xfer[0].tx_buf)[0]);
>>
>> Is the cast needed ?
> 
> ?

Sorry, I don't understand your question.
To rephrase my original question, is the (u8 *) cast needed ?

>> > + � rpc->smenr = RPC_SMENR_CDE | RPC_SMENR_CDB(fls(xfer[0].tx_nbits
>>> 1));
>> > + � rpc->addr = 0;
>> > +
>> > + � if (xfercnt > 2 && xfer[1].len && xfer[1].tx_buf) {
>> > + � � �rpc->smenr |= RPC_SMENR_ADB(fls(xfer[1].tx_nbits >> 1));
>> > + � � �for (i = 0; i < xfer[1].len; i++)
>> > + � � � � rpc->addr |= (u32)((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);
>> > + � }
>> > +
>> > + � switch (xfercnt) {
>> > + � case 2:
>> > + � � �if (xfer[1].rx_buf) {
>> > + � � � � rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer
>> > + � � � � � � � � �(xfer[1].len)) | RPC_SMENR_SPIDB(fls
>> > + � � � � � � � � �(xfer[1].rx_nbits >> 1));
>>
>> How much of this register value calculation could be somehow
>> deduplicated ? It seems to be almost the same thing copied thrice here.
> 
> I don't get your point!
> 
> The 2'nd transfer may be
> 1) spi-address
> 2) tx_buf[] for write registers.
> 3) rx_buf[] for read status.
> 
> parse them and write to rpc->addr and so on.
> Or you have a better way to do this ?

Each of the case statement options has almost the same stuff in it. Can
this be somehow reworked so that it wouldn't be three copies of almost
the same ?

[...]

-- 
Best regards,
Marek Vasut

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

[Marek Vasut]

On 11/23/2018 01:45 AM, masonccyang@mxic.com.tw wrote:
> Hi Marek,

Hi,

>> > +
>> > +struct rpc_spi {
>> > +   struct clk *clk_rpc;
>> > +   void __iomem *regs;
>> > +   struct {
>> > +      void __iomem *map;
>> > +      dma_addr_t dma;
>> > +      size_t size;
>> > +   } linear;
>>
>> Does this need it's own struct ?
>>
> 
> yup, I think it's better.
> In case no "dirmap" in dtb and no direct mapping mode implemented.
> 
> 
>> > +   u32 cur_speed_hz;
>> > +   u32 cmd;
>> > +   u32 addr;
>> > +   u32 dummy;
>> > +   u32 smcr;
>> > +   u32 smenr;
>> > +   u32 xferlen;
>> > +   u32 totalxferlen;
>>
>> This register cache might be a good candidate for regmap ?
> 
> I don't know what does it mean ?
> Could you give me more information!

See include/linux/regmap.h and git grep regmap drivers/ for examples.

>> > +   enum spi_mem_data_dir xfer_dir;
>> > +};
>> > +
>> > +static int rpc_spi_set_freq(struct rpc_spi *rpc, unsigned long freq)
>> > +{
>> > +   int ret;
>> > +
>> > +   if (rpc->cur_speed_hz == freq)
>> > +      return 0;
>> > +
>> > +   clk_disable_unprepare(rpc->clk_rpc);
>> > +   ret = clk_set_rate(rpc->clk_rpc, freq);
>> > +   if (ret)
>> > +      return ret;
>> > +
>> > +   ret = clk_prepare_enable(rpc->clk_rpc);
>> > +   if (ret)
>> > +      return ret;
>>
>> Is this clock disable/update/enable really needed ? I'd think that
>> clk_set_rate() would handle the rate update correctly.
> 
> As Gerrt mentioned, I will remove them.
> 
> 
>> > +static int wait_msg_xfer_end(struct rpc_spi *rpc)
>> > +{
>> > +   u32 sts;
>> > +
>> > +   return readl_poll_timeout(rpc->regs + RPC_CMNSR, sts,
>> > +              sts & RPC_CMNSR_TEND, 0, USEC_PER_SEC);
>> > +}
>> > +
>> > +static u8 rpc_bits_xfer(u32 nbytes)
>> > +{
>> > +   u8 databyte;
>> > +
>> > +   switch (nbytes) {
>>
>> Did you ever test unaligned writes and reads ? There are some nasty edge
>> cases in those.
>>
>> Also, I think you can calculate the number of set bits using a simple
>> function, so the switch-case might not even be needed.
>>
> 
> Any example function ?

Nope, you'd have to think of one. You need to fill $nbytes bits from top
down. I think you can somehow use GENMASK() .

>> > +   case 1:
>> > +      databyte = 0x8;
>> > +      break;
>> > +   case 2:
>> > +      databyte = 0xc;
>> > +      break;
>> > +   default:
>> > +      databyte = 0xf;
>> > +      break;
>> > +   }
>> > +
>> > +   return databyte;
>> > +}
>> > +
>> > +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;
>> > +
>> > +   writel(RPC_CMNCR_MD | RPC_CMNCR_SFDE | RPC_CMNCR_MOIIO_HIZ |
>> > +          RPC_CMNCR_IOFV_HIZ | RPC_CMNCR_BSZ(0), rpc->regs +
> RPC_CMNCR);
>> > +   writel(0x0, rpc->regs + RPC_SMDRENR);
>> > +
>> > +   if (tx_buf) {
>> > +      writel(rpc->cmd, rpc->regs + RPC_SMCMR);
>> > +      writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
>> > +      writel(rpc->addr, rpc->regs + RPC_SMADR);
>> > +      smenr = rpc->smenr;
>> > +
>> > +      while (pos < rpc->xferlen) {
>> > +         u32 nbytes = rpc->xferlen  - pos;
>> > +
>> > +         writel(*(u32 *)(tx_buf + pos), rpc->regs + RPC_SMWDR0);
>> > +
>> > +         if (nbytes > 4) {
>> > +            nbytes = 4;
>> > +            smcr = rpc->smcr |
>> > +                   RPC_SMCR_SPIE | RPC_SMCR_SSLKP;
>> > +         } else {
>> > +            smcr = rpc->smcr | RPC_SMCR_SPIE;
>> > +         }
>> > +
>> > +         writel(smenr, rpc->regs + RPC_SMENR);
>> > +         writel(smcr, rpc->regs + RPC_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) {
>> > +      while (pos < rpc->xferlen) {
>> > +         u32 nbytes = rpc->xferlen  - pos;
>> > +
>> > +         if (nbytes > 4)
>> > +            nbytes = 4;
>> > +
>> > +         writel(rpc->cmd, rpc->regs + RPC_SMCMR);
>> > +         writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
>> > +         writel(rpc->addr + pos, rpc->regs + RPC_SMADR);
>> > +         writel(rpc->smenr, rpc->regs + RPC_SMENR);
>> > +         writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
>> > +         ret = wait_msg_xfer_end(rpc);
>> > +         if (ret)
>> > +            goto out;
>> > +
>> > +         data = readl(rpc->regs + RPC_SMRDR0);
>> > +         memcpy_fromio(rx_buf + pos, (void *)&data, nbytes);
>> > +         pos += nbytes;
>> > +      }
>> > +   } else {
>> > +      writel(rpc->cmd, rpc->regs + RPC_SMCMR);
>> > +      writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
>> > +      writel(rpc->addr + pos, rpc->regs + RPC_SMADR);
>> > +      writel(rpc->smenr, rpc->regs + RPC_SMENR);
>> > +      writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
>> > +      ret = wait_msg_xfer_end(rpc);
>> > +   }
>> > +out:
>>
>> Dont you need to stop the RPC somehow in case the transmission fails ?
>>
> 
> I can't find any RPC registers can do this !
> 
> Do you know how to do this ?

It should be in the RPC datasheet ? It's likely going to involve SMCR,
possibly clear SPIE bit and maybe some more.

>> > +   writel(rpc->cmd, rpc->regs + RPC_SMCMR);
>> > +   writel(offs, rpc->regs + RPC_SMADR);
>> > +   writel(rpc->smenr, rpc->regs + RPC_SMENR);
>> > +   writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
>> > +   ret = wait_msg_xfer_end(rpc);
>> > +   if (ret)
>> > +      goto out;
>> > +
>> > +   writel(RPC_DRCR_RCF, rpc->regs + RPC_DRCR);
>> > +   writel(RPC_PHYCNT_CAL | RPC_PHYCNT_STRTIM(0) | 0x260,
>> > +          rpc->regs + RPC_PHYCNT);
>> > +
>> > +   return len;
>> > +out:
>>
>> Shouldn't you shut the controller down if the xfer fails ?
> 
> Any registers can shut down RPC controller ?
> SW reset ?

Possibly, can you research it ?

>> > +   return ret;
>> > +}
>> > +
>> > +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->linear.map &&
>> > +       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;
>> > +   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;
>> > +      }
>> > +   }
>> > +
>> > +   xfercnt = xferpos;
>> > +   rpc->xferlen = xfer[--xferpos].len;
>> > +   rpc->cmd = RPC_SMCMR_CMD(((u8 *)xfer[0].tx_buf)[0]);
>>
>> Is the cast needed ?
> 
> ?

Sorry, I don't understand your question.
To rephrase my original question, is the (u8 *) cast needed ?

>> > +   rpc->smenr = RPC_SMENR_CDE | RPC_SMENR_CDB(fls(xfer[0].tx_nbits
>>> 1));
>> > +   rpc->addr = 0;
>> > +
>> > +   if (xfercnt > 2 && xfer[1].len && xfer[1].tx_buf) {
>> > +      rpc->smenr |= RPC_SMENR_ADB(fls(xfer[1].tx_nbits >> 1));
>> > +      for (i = 0; i < xfer[1].len; i++)
>> > +         rpc->addr |= (u32)((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);
>> > +   }
>> > +
>> > +   switch (xfercnt) {
>> > +   case 2:
>> > +      if (xfer[1].rx_buf) {
>> > +         rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer
>> > +                  (xfer[1].len)) | RPC_SMENR_SPIDB(fls
>> > +                  (xfer[1].rx_nbits >> 1));
>>
>> How much of this register value calculation could be somehow
>> deduplicated ? It seems to be almost the same thing copied thrice here.
> 
> I don't get your point!
> 
> The 2'nd transfer may be
> 1) spi-address
> 2) tx_buf[] for write registers.
> 3) rx_buf[] for read status.
> 
> parse them and write to rpc->addr and so on.
> Or you have a better way to do this ?

Each of the case statement options has almost the same stuff in it. Can
this be somehow reworked so that it wouldn't be three copies of almost
the same ?

[...]

-- 
Best regards,
Marek Vasut

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 10563 bytes --]

Hi Mason,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on spi/for-next]
[also build test ERROR on v4.20-rc3 next-20181119]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Mason-Yang/spi-Add-Renesas-R-Car-RPC-SPI-controller-driver/20181120-020310
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All error/warnings (new ones prefixed by >>):

>> drivers/spi/spi-renesas-rpc.c:366:47: warning: 'struct spi_mem_dirmap_desc' declared inside parameter list will not be visible outside of this definition or declaration
    static ssize_t rpc_spi_mem_dirmap_read(struct spi_mem_dirmap_desc *desc,
                                                  ^~~~~~~~~~~~~~~~~~~
   In file included from drivers/spi/spi-renesas-rpc.c:18:0:
   drivers/spi/spi-renesas-rpc.c: In function 'rpc_spi_mem_dirmap_read':
>> drivers/spi/spi-renesas-rpc.c:369:51: error: dereferencing pointer to incomplete type 'struct spi_mem_dirmap_desc'
     struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
                                                      ^
   include/linux/spi/spi.h:1333:66: note: in definition of macro 'spi_master_get_devdata'
    #define spi_master_get_devdata(_ctlr) spi_controller_get_devdata(_ctlr)
                                                                     ^~~~~
   drivers/spi/spi-renesas-rpc.c: At top level:
   drivers/spi/spi-renesas-rpc.c:397:48: warning: 'struct spi_mem_dirmap_desc' declared inside parameter list will not be visible outside of this definition or declaration
    static ssize_t rpc_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
                                                   ^~~~~~~~~~~~~~~~~~~
   In file included from drivers/spi/spi-renesas-rpc.c:18:0:
   drivers/spi/spi-renesas-rpc.c: In function 'rpc_spi_mem_dirmap_write':
   drivers/spi/spi-renesas-rpc.c:400:51: error: dereferencing pointer to incomplete type 'struct spi_mem_dirmap_desc'
     struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
                                                      ^
   include/linux/spi/spi.h:1333:66: note: in definition of macro 'spi_master_get_devdata'
    #define spi_master_get_devdata(_ctlr) spi_controller_get_devdata(_ctlr)
                                                                     ^~~~~
   drivers/spi/spi-renesas-rpc.c: At top level:
   drivers/spi/spi-renesas-rpc.c:443:45: warning: 'struct spi_mem_dirmap_desc' declared inside parameter list will not be visible outside of this definition or declaration
    static int rpc_spi_mem_dirmap_create(struct spi_mem_dirmap_desc *desc)
                                                ^~~~~~~~~~~~~~~~~~~
   In file included from drivers/spi/spi-renesas-rpc.c:18:0:
   drivers/spi/spi-renesas-rpc.c: In function 'rpc_spi_mem_dirmap_create':
   drivers/spi/spi-renesas-rpc.c:445:51: error: dereferencing pointer to incomplete type 'struct spi_mem_dirmap_desc'
     struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
                                                      ^
   include/linux/spi/spi.h:1333:66: note: in definition of macro 'spi_master_get_devdata'
    #define spi_master_get_devdata(_ctlr) spi_controller_get_devdata(_ctlr)
                                                                     ^~~~~
   drivers/spi/spi-renesas-rpc.c: At top level:
>> drivers/spi/spi-renesas-rpc.c:484:3: error: 'const struct spi_controller_mem_ops' has no member named 'dirmap_create'
     .dirmap_create = rpc_spi_mem_dirmap_create,
      ^~~~~~~~~~~~~
>> drivers/spi/spi-renesas-rpc.c:484:19: error: positional initialization of field in 'struct' declared with 'designated_init' attribute [-Werror=designated-init]
     .dirmap_create = rpc_spi_mem_dirmap_create,
                      ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/spi/spi-renesas-rpc.c:484:19: note: (near initialization for 'rpc_spi_mem_ops')
>> drivers/spi/spi-renesas-rpc.c:484:19: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
   drivers/spi/spi-renesas-rpc.c:484:19: note: (near initialization for 'rpc_spi_mem_ops.supports_op')
>> drivers/spi/spi-renesas-rpc.c:485:3: error: 'const struct spi_controller_mem_ops' has no member named 'dirmap_read'
     .dirmap_read = rpc_spi_mem_dirmap_read,
      ^~~~~~~~~~~
   drivers/spi/spi-renesas-rpc.c:485:17: error: positional initialization of field in 'struct' declared with 'designated_init' attribute [-Werror=designated-init]
     .dirmap_read = rpc_spi_mem_dirmap_read,
                    ^~~~~~~~~~~~~~~~~~~~~~~
   drivers/spi/spi-renesas-rpc.c:485:17: note: (near initialization for 'rpc_spi_mem_ops')
   drivers/spi/spi-renesas-rpc.c:485:17: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
   drivers/spi/spi-renesas-rpc.c:485:17: note: (near initialization for 'rpc_spi_mem_ops.adjust_op_size')
>> drivers/spi/spi-renesas-rpc.c:486:3: error: 'const struct spi_controller_mem_ops' has no member named 'dirmap_write'
     .dirmap_write = rpc_spi_mem_dirmap_write,
      ^~~~~~~~~~~~
   drivers/spi/spi-renesas-rpc.c:486:18: error: positional initialization of field in 'struct' declared with 'designated_init' attribute [-Werror=designated-init]
     .dirmap_write = rpc_spi_mem_dirmap_write,
                     ^~~~~~~~~~~~~~~~~~~~~~~~
   drivers/spi/spi-renesas-rpc.c:486:18: note: (near initialization for 'rpc_spi_mem_ops')
   drivers/spi/spi-renesas-rpc.c:486:18: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
   drivers/spi/spi-renesas-rpc.c:486:18: note: (near initialization for 'rpc_spi_mem_ops.get_name')
   cc1: some warnings being treated as errors

vim +369 drivers/spi/spi-renesas-rpc.c

   365	
 > 366	static ssize_t rpc_spi_mem_dirmap_read(struct spi_mem_dirmap_desc *desc,
   367					       u64 offs, size_t len, void *buf)
   368	{
 > 369		struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
   370		int ret;
   371	
   372		if (WARN_ON(offs + desc->info.offset + len > U32_MAX))
   373			return -EINVAL;
   374	
   375		ret = rpc_spi_set_freq(rpc, desc->mem->spi->max_speed_hz);
   376		if (ret)
   377			return ret;
   378	
   379		rpc_spi_mem_set_prep_op_cfg(desc->mem->spi,
   380					    &desc->info.op_tmpl, &offs, &len);
   381	
   382		writel(RPC_CMNCR_SFDE | RPC_CMNCR_MOIIO_HIZ |
   383		       RPC_CMNCR_IOFV_HIZ | RPC_CMNCR_BSZ(0), rpc->regs + RPC_CMNCR);
   384	
   385		writel(RPC_DRCR_RBURST(0x1f) | RPC_DRCR_RBE, rpc->regs + RPC_DRCR);
   386		writel(rpc->cmd, rpc->regs + RPC_DRCMR);
   387		writel(RPC_DREAR_EAC, rpc->regs + RPC_DREAR);
   388		writel(0, rpc->regs + RPC_DROPR);
   389		writel(rpc->smenr, rpc->regs + RPC_DRENR);
   390		writel(rpc->dummy, rpc->regs + RPC_DRDMCR);
   391		writel(0x0, rpc->regs + RPC_DRDRENR);
   392		memcpy_fromio(buf, rpc->linear.map + desc->info.offset + offs, len);
   393	
   394		return len;
   395	}
   396	
   397	static ssize_t rpc_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
   398						u64 offs, size_t len, const void *buf)
   399	{
   400		struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
   401		int tx_offs, ret;
   402	
   403		if (WARN_ON(offs + desc->info.offset + len > U32_MAX))
   404			return -EINVAL;
   405	
   406		if (WARN_ON(len > RPC_WBUF_SIZE))
   407			return -EIO;
   408	
   409		ret = rpc_spi_set_freq(rpc, desc->mem->spi->max_speed_hz);
   410		if (ret)
   411			return ret;
   412	
   413		rpc_spi_mem_set_prep_op_cfg(desc->mem->spi,
   414					    &desc->info.op_tmpl, &offs, &len);
   415	
   416		writel(RPC_CMNCR_MD | RPC_CMNCR_SFDE | RPC_CMNCR_MOIIO_HIZ |
   417		       RPC_CMNCR_IOFV_HIZ | RPC_CMNCR_BSZ(0), rpc->regs + RPC_CMNCR);
   418		writel(0x0, rpc->regs + RPC_SMDRENR);
   419	
   420		writel(RPC_PHYCNT_CAL | 0x260 | RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF,
   421		       rpc->regs + RPC_PHYCNT);
   422	
   423		for (tx_offs = 0; tx_offs < RPC_WBUF_SIZE; tx_offs += 4)
   424			writel(*(u32 *)(buf + tx_offs), rpc->regs + RPC_WBUF + tx_offs);
   425	
   426		writel(rpc->cmd, rpc->regs + RPC_SMCMR);
   427		writel(offs, rpc->regs + RPC_SMADR);
   428		writel(rpc->smenr, rpc->regs + RPC_SMENR);
   429		writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
   430		ret = wait_msg_xfer_end(rpc);
   431		if (ret)
   432			goto out;
   433	
   434		writel(RPC_DRCR_RCF, rpc->regs + RPC_DRCR);
   435		writel(RPC_PHYCNT_CAL | RPC_PHYCNT_STRTIM(0) | 0x260,
   436		       rpc->regs + RPC_PHYCNT);
   437	
   438		return len;
   439	out:
   440		return ret;
   441	}
   442	
   443	static int rpc_spi_mem_dirmap_create(struct spi_mem_dirmap_desc *desc)
   444	{
   445		struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
   446	
   447		if (desc->info.offset + desc->info.length > U32_MAX)
   448			return -ENOTSUPP;
   449	
   450		if (!rpc_spi_mem_supports_op(desc->mem, &desc->info.op_tmpl))
   451			return -ENOTSUPP;
   452	
   453		if (!rpc->linear.map &&
   454		    desc->info.op_tmpl.data.dir == SPI_MEM_DATA_IN)
   455			return -ENOTSUPP;
   456	
   457		return 0;
   458	}
   459	
   460	static int rpc_spi_mem_exec_op(struct spi_mem *mem,
   461				       const struct spi_mem_op *op)
   462	{
   463		struct rpc_spi *rpc = spi_master_get_devdata(mem->spi->master);
   464		int ret;
   465	
   466		ret = rpc_spi_set_freq(rpc, mem->spi->max_speed_hz);
   467		if (ret)
   468			return ret;
   469	
   470		rpc_spi_mem_set_prep_op_cfg(mem->spi, op, NULL, NULL);
   471	
   472		ret = rpc_spi_io_xfer(rpc,
   473				      op->data.dir == SPI_MEM_DATA_OUT ?
   474				      op->data.buf.out : NULL,
   475				      op->data.dir == SPI_MEM_DATA_IN ?
   476				      op->data.buf.in : NULL);
   477	
   478		return ret;
   479	}
   480	
   481	static const struct spi_controller_mem_ops rpc_spi_mem_ops = {
   482		.supports_op = rpc_spi_mem_supports_op,
   483		.exec_op = rpc_spi_mem_exec_op,
 > 484		.dirmap_create = rpc_spi_mem_dirmap_create,
 > 485		.dirmap_read = rpc_spi_mem_dirmap_read,
 > 486		.dirmap_write = rpc_spi_mem_dirmap_write,
   487	};
   488	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 66042 bytes --]

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5867 bytes --]

Hi Mason,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on spi/for-next]
[also build test WARNING on v4.20-rc3 next-20181119]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Mason-Yang/spi-Add-Renesas-R-Car-RPC-SPI-controller-driver/20181120-020310
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: sh-allyesconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=sh 

All warnings (new ones prefixed by >>):

   drivers//spi/spi-renesas-rpc.c:366:47: warning: 'struct spi_mem_dirmap_desc' declared inside parameter list will not be visible outside of this definition or declaration
    static ssize_t rpc_spi_mem_dirmap_read(struct spi_mem_dirmap_desc *desc,
                                                  ^~~~~~~~~~~~~~~~~~~
   In file included from drivers//spi/spi-renesas-rpc.c:18:0:
   drivers//spi/spi-renesas-rpc.c: In function 'rpc_spi_mem_dirmap_read':
   drivers//spi/spi-renesas-rpc.c:369:51: error: dereferencing pointer to incomplete type 'struct spi_mem_dirmap_desc'
     struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
                                                      ^
   include/linux/spi/spi.h:1333:66: note: in definition of macro 'spi_master_get_devdata'
    #define spi_master_get_devdata(_ctlr) spi_controller_get_devdata(_ctlr)
                                                                     ^~~~~
   drivers//spi/spi-renesas-rpc.c: At top level:
   drivers//spi/spi-renesas-rpc.c:397:48: warning: 'struct spi_mem_dirmap_desc' declared inside parameter list will not be visible outside of this definition or declaration
    static ssize_t rpc_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
                                                   ^~~~~~~~~~~~~~~~~~~
   In file included from drivers//spi/spi-renesas-rpc.c:18:0:
   drivers//spi/spi-renesas-rpc.c: In function 'rpc_spi_mem_dirmap_write':
   drivers//spi/spi-renesas-rpc.c:400:51: error: dereferencing pointer to incomplete type 'struct spi_mem_dirmap_desc'
     struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
                                                      ^
   include/linux/spi/spi.h:1333:66: note: in definition of macro 'spi_master_get_devdata'
    #define spi_master_get_devdata(_ctlr) spi_controller_get_devdata(_ctlr)
                                                                     ^~~~~
   drivers//spi/spi-renesas-rpc.c: At top level:
   drivers//spi/spi-renesas-rpc.c:443:45: warning: 'struct spi_mem_dirmap_desc' declared inside parameter list will not be visible outside of this definition or declaration
    static int rpc_spi_mem_dirmap_create(struct spi_mem_dirmap_desc *desc)
                                                ^~~~~~~~~~~~~~~~~~~
   In file included from drivers//spi/spi-renesas-rpc.c:18:0:
   drivers//spi/spi-renesas-rpc.c: In function 'rpc_spi_mem_dirmap_create':
   drivers//spi/spi-renesas-rpc.c:445:51: error: dereferencing pointer to incomplete type 'struct spi_mem_dirmap_desc'
     struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
                                                      ^
   include/linux/spi/spi.h:1333:66: note: in definition of macro 'spi_master_get_devdata'
    #define spi_master_get_devdata(_ctlr) spi_controller_get_devdata(_ctlr)
                                                                     ^~~~~
   drivers//spi/spi-renesas-rpc.c: At top level:
   drivers//spi/spi-renesas-rpc.c:484:3: error: 'const struct spi_controller_mem_ops' has no member named 'dirmap_create'
     .dirmap_create = rpc_spi_mem_dirmap_create,
      ^~~~~~~~~~~~~
   drivers//spi/spi-renesas-rpc.c:484:19: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
     .dirmap_create = rpc_spi_mem_dirmap_create,
                      ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers//spi/spi-renesas-rpc.c:484:19: note: (near initialization for 'rpc_spi_mem_ops.get_name')
   drivers//spi/spi-renesas-rpc.c:485:3: error: 'const struct spi_controller_mem_ops' has no member named 'dirmap_read'
     .dirmap_read = rpc_spi_mem_dirmap_read,
      ^~~~~~~~~~~
>> drivers//spi/spi-renesas-rpc.c:485:17: warning: excess elements in struct initializer
     .dirmap_read = rpc_spi_mem_dirmap_read,
                    ^~~~~~~~~~~~~~~~~~~~~~~
   drivers//spi/spi-renesas-rpc.c:485:17: note: (near initialization for 'rpc_spi_mem_ops')
   drivers//spi/spi-renesas-rpc.c:486:3: error: 'const struct spi_controller_mem_ops' has no member named 'dirmap_write'
     .dirmap_write = rpc_spi_mem_dirmap_write,
      ^~~~~~~~~~~~
   drivers//spi/spi-renesas-rpc.c:486:18: warning: excess elements in struct initializer
     .dirmap_write = rpc_spi_mem_dirmap_write,
                     ^~~~~~~~~~~~~~~~~~~~~~~~
   drivers//spi/spi-renesas-rpc.c:486:18: note: (near initialization for 'rpc_spi_mem_ops')
   cc1: some warnings being treated as errors

vim +485 drivers//spi/spi-renesas-rpc.c

   480	
   481	static const struct spi_controller_mem_ops rpc_spi_mem_ops = {
   482		.supports_op = rpc_spi_mem_supports_op,
   483		.exec_op = rpc_spi_mem_exec_op,
 > 484		.dirmap_create = rpc_spi_mem_dirmap_create,
 > 485		.dirmap_read = rpc_spi_mem_dirmap_read,
   486		.dirmap_write = rpc_spi_mem_dirmap_write,
   487	};
   488	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 50583 bytes --]

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

[Geert Uytterhoeven]

Hi Mason,

On Mon, Nov 19, 2018 at 11:01 AM Mason Yang <masonccyang@mxic.com.tw> wrote:
> Add a driver for Renesas R-Car D3 RPC SPI controller driver.
>
> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>

Thanks for your patch!

> --- /dev/null
> +++ b/drivers/spi/spi-renesas-rpc.c
> @@ -0,0 +1,750 @@

> +static int rpc_spi_set_freq(struct rpc_spi *rpc, unsigned long freq)
> +{
> +       int ret;
> +
> +       if (rpc->cur_speed_hz == freq)
> +               return 0;
> +
> +       clk_disable_unprepare(rpc->clk_rpc);
> +       ret = clk_set_rate(rpc->clk_rpc, freq);
> +       if (ret)
> +               return ret;
> +
> +       ret = clk_prepare_enable(rpc->clk_rpc);
> +       if (ret)
> +               return ret;

The clk_{disable_unprepare,prepare_enable}() may be needed on the Macronix
controller you based this driver on, but will be futile on Renesas SoCs.

As the RPC is part of the CPG/MSSR clock domain, its clock will be controlled
by the Runtime PM.  As you've already called pm_runtime_get_sync() from your
.probe() calback, Runtime PM will have enabled the clock.
If you disable it manually, you create an imbalance between automatic and
manual clock control.

So please don't control the clock explicitly, but always use
pm_runtime_*() calls.

> +static int __maybe_unused rpc_spi_runtime_suspend(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct spi_master *master = platform_get_drvdata(pdev);
> +       struct rpc_spi *rpc = spi_master_get_devdata(master);
> +
> +       clk_disable_unprepare(rpc->clk_rpc);

At this point, the clock is enabled due to Runtime PM, and you disable it
manually.
During system suspend, the clock will be disabled by the PM framework
again, leading to a negative enable count.
I expect you to see warning splats during system suspend.
Hence please drop the explicit clock management from this function.

I'm not familiar with the spimem framework, but for a normal SPI controller,
you want to call spi_master_resume(master) here.
See e.g. commit c1ca59c22c56930b ("spi: rspi: Fix invalid SPI use during
system suspend")

> +
> +       return 0;
> +}
> +
> +static int __maybe_unused rpc_spi_runtime_resume(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct spi_master *master = platform_get_drvdata(pdev);
> +       struct rpc_spi *rpc = spi_master_get_devdata(master);
> +       int ret;
> +
> +       ret = clk_prepare_enable(rpc->clk_rpc);
> +       if (ret)
> +               dev_err(dev, "Can't enable rpc->clk_rpc\n");

Likewise, please drop the explicit clock management here. The PM core
code will handle it through the clock domain.

+ spi_master_resume(master)

> +
> +       return ret;
> +}
> +
> +static const struct dev_pm_ops rpc_spi_dev_pm_ops = {
> +       SET_RUNTIME_PM_OPS(rpc_spi_runtime_suspend,
> +                          rpc_spi_runtime_resume, NULL)

Ah, you only use these for Runtime PM.  Not needed, as Runtime PM handles
the clock domain without any callbacks.

With spi_master_{suspend,resume}() added, you can use SIMPLE_DEV_PM_OPS(),
and make everything work during/after system suspend.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

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

[Boris Brezillon]

On Tue, 20 Nov 2018 09:01:29 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > --- /dev/null
> > +++ b/drivers/spi/spi-renesas-rpc.c
> > @@ -0,0 +1,750 @@  
> 
> > +static int rpc_spi_set_freq(struct rpc_spi *rpc, unsigned long freq)
> > +{
> > +       int ret;
> > +
> > +       if (rpc->cur_speed_hz == freq)
> > +               return 0;
> > +
> > +       clk_disable_unprepare(rpc->clk_rpc);
> > +       ret = clk_set_rate(rpc->clk_rpc, freq);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = clk_prepare_enable(rpc->clk_rpc);
> > +       if (ret)
> > +               return ret;  
> 
> The clk_{disable_unprepare,prepare_enable}() may be needed on the Macronix
> controller you based this driver on, but will be futile on Renesas SoCs.
> 
> As the RPC is part of the CPG/MSSR clock domain, its clock will be controlled
> by the Runtime PM.  As you've already called pm_runtime_get_sync() from your
> .probe() calback, Runtime PM will have enabled the clock.
> If you disable it manually, you create an imbalance between automatic and
> manual clock control.
> 
> So please don't control the clock explicitly, but always use
> pm_runtime_*() calls.

More about that. The reason we did that on MXIC is that the clk rate
can't be changed when the clk is enabled. So we have to

1/ explicitly disable the clk that has been enabled by runtime PM
2/ set the new rate
3/ re-enable the clk

So the clk enable/disable are not unbalanced, but it's also true that
this disable/set_rate/enable dance might be unneeded on your platform.

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

[masonccyang]

[-- Attachment #1: Type: text/plain, Size: 4032 bytes --]

Hi Boris,

Got it.
Thanks for your explanation.

best regards,
Msaon


> Boris Brezillon <boris.brezillon@bootlin.com> 
> 2018/11/20 下午 04:11
> 
> To
> 
> Geert Uytterhoeven <geert@linux-m68k.org>, 
> 
> cc
> 
> masonccyang@mxic.com.tw, Mark Brown <broonie@kernel.org>, Trent 
> Piepho <tpiepho@impinj.com>, Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>, linux-spi <linux-spi@vger.kernel.org>, 
> Linux-Renesas <linux-renesas-soc@vger.kernel.org>, Simon Horman 
> <horms@verge.net.au>, juliensu@mxic.com.tw, Geert Uytterhoeven 
> <geert+renesas@glider.be>, zhengxunli@mxic.com.tw
> 
> Subject
> 
> Re: [PATCH 1/2] spi: Add Renesas R-Car RPC SPI controller driver
> 
> On Tue, 20 Nov 2018 09:01:29 +0100
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 
> > > --- /dev/null
> > > +++ b/drivers/spi/spi-renesas-rpc.c
> > > @@ -0,0 +1,750 @@ 
> > 
> > > +static int rpc_spi_set_freq(struct rpc_spi *rpc, unsigned long 
freq)
> > > +{
> > > +       int ret;
> > > +
> > > +       if (rpc->cur_speed_hz == freq)
> > > +               return 0;
> > > +
> > > +       clk_disable_unprepare(rpc->clk_rpc);
> > > +       ret = clk_set_rate(rpc->clk_rpc, freq);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       ret = clk_prepare_enable(rpc->clk_rpc);
> > > +       if (ret)
> > > +               return ret; 
> > 
> > The clk_{disable_unprepare,prepare_enable}() may be needed on the 
Macronix
> > controller you based this driver on, but will be futile on Renesas 
SoCs.
> > 
> > As the RPC is part of the CPG/MSSR clock domain, its clock will 
becontrolled
> > by the Runtime PM.  As you've already called pm_runtime_get_sync()from 
your
> > .probe() calback, Runtime PM will have enabled the clock.
> > If you disable it manually, you create an imbalance between automatic 
and
> > manual clock control.
> > 
> > So please don't control the clock explicitly, but always use
> > pm_runtime_*() calls.
> 
> More about that. The reason we did that on MXIC is that the clk rate
> can't be changed when the clk is enabled. So we have to
> 
> 1/ explicitly disable the clk that has been enabled by runtime PM
> 2/ set the new rate
> 3/ re-enable the clk
> 
> So the clk enable/disable are not unbalanced, but it's also true that
> this disable/set_rate/enable dance might be unneeded on your platform.


CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

[-- Attachment #2: Type: text/html, Size: 4560 bytes --]

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

[masonccyang]

[-- Attachment #1: Type: text/plain, Size: 6338 bytes --]

Hi Geert,

Thanks for your review and comments.

I will patch RPC driver and it's dt-binding.

best regards,
Mason


Geert Uytterhoeven <geert@linux-m68k.org> 已在 2018/11/20 下午 04:01:29 上
寫入: 

> Geert Uytterhoeven <geert@linux-m68k.org> 
> 2018/11/20 下午 04:01
> 
> To
> 
> masonccyang@mxic.com.tw, 
> 
> cc
> 
> Mark Brown <broonie@kernel.org>, Trent Piepho <tpiepho@impinj.com>, 
> Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, linux-spi 
> <linux-spi@vger.kernel.org>, Linux-Renesas <linux-renesas-
> soc@vger.kernel.org>, Simon Horman <horms@verge.net.au>, Boris 
> Brezillon <boris.brezillon@bootlin.com>, juliensu@mxic.com.tw, Geert
> Uytterhoeven <geert+renesas@glider.be>, zhengxunli@mxic.com.tw
> 
> Subject
> 
> Re: [PATCH 1/2] spi: Add Renesas R-Car RPC SPI controller driver
> 
> Hi Mason,
> 
> On Mon, Nov 19, 2018 at 11:01 AM Mason Yang <masonccyang@mxic.com.tw> 
wrote:
> > Add a driver for Renesas R-Car D3 RPC SPI controller driver.
> >
> > Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> 
> Thanks for your patch!
> 
> > --- /dev/null
> > +++ b/drivers/spi/spi-renesas-rpc.c
> > @@ -0,0 +1,750 @@
> 
> > +static int rpc_spi_set_freq(struct rpc_spi *rpc, unsigned long freq)
> > +{
> > +       int ret;
> > +
> > +       if (rpc->cur_speed_hz == freq)
> > +               return 0;
> > +
> > +       clk_disable_unprepare(rpc->clk_rpc);
> > +       ret = clk_set_rate(rpc->clk_rpc, freq);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = clk_prepare_enable(rpc->clk_rpc);
> > +       if (ret)
> > +               return ret;
> 
> The clk_{disable_unprepare,prepare_enable}() may be needed on the 
Macronix
> controller you based this driver on, but will be futile on Renesas SoCs.
> 
> As the RPC is part of the CPG/MSSR clock domain, its clock will be 
controlled
> by the Runtime PM.  As you've already called pm_runtime_get_sync() from 
your
> .probe() calback, Runtime PM will have enabled the clock.
> If you disable it manually, you create an imbalance between automatic 
and
> manual clock control.
> 
> So please don't control the clock explicitly, but always use
> pm_runtime_*() calls.
> 
> > +static int __maybe_unused rpc_spi_runtime_suspend(struct device *dev)
> > +{
> > +       struct platform_device *pdev = to_platform_device(dev);
> > +       struct spi_master *master = platform_get_drvdata(pdev);
> > +       struct rpc_spi *rpc = spi_master_get_devdata(master);
> > +
> > +       clk_disable_unprepare(rpc->clk_rpc);
> 
> At this point, the clock is enabled due to Runtime PM, and you disable 
it
> manually.
> During system suspend, the clock will be disabled by the PM framework
> again, leading to a negative enable count.
> I expect you to see warning splats during system suspend.
> Hence please drop the explicit clock management from this function.
> 
> I'm not familiar with the spimem framework, but for a normal SPI 
controller,
> you want to call spi_master_resume(master) here.
> See e.g. commit c1ca59c22c56930b ("spi: rspi: Fix invalid SPI use during
> system suspend")
> 
> > +
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused rpc_spi_runtime_resume(struct device *dev)
> > +{
> > +       struct platform_device *pdev = to_platform_device(dev);
> > +       struct spi_master *master = platform_get_drvdata(pdev);
> > +       struct rpc_spi *rpc = spi_master_get_devdata(master);
> > +       int ret;
> > +
> > +       ret = clk_prepare_enable(rpc->clk_rpc);
> > +       if (ret)
> > +               dev_err(dev, "Can't enable rpc->clk_rpc\n");
> 
> Likewise, please drop the explicit clock management here. The PM core
> code will handle it through the clock domain.
> 
> + spi_master_resume(master)
> 
> > +
> > +       return ret;
> > +}
> > +
> > +static const struct dev_pm_ops rpc_spi_dev_pm_ops = {
> > +       SET_RUNTIME_PM_OPS(rpc_spi_runtime_suspend,
> > +                          rpc_spi_runtime_resume, NULL)
> 
> Ah, you only use these for Runtime PM.  Not needed, as Runtime PM 
handles
> the clock domain without any callbacks.
> 
> With spi_master_{suspend,resume}() added, you can use 
SIMPLE_DEV_PM_OPS(),
> and make everything work during/after system suspend.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. 
But
> when I'm talking to journalists I just say "programmer" or somethinglike 
that.
>                                 -- Linus Torvalds


CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

[-- Attachment #2: Type: text/html, Size: 7788 bytes --]

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

[Mason Yang]

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

Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
---
 .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
 1 file changed, 33 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..8286cc8
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
@@ -0,0 +1,33 @@
+Renesas R-Car D3 RPC controller Device Tree Bindings
+----------------------------------------------------
+
+Required properties:
+- compatible: should be "renesas,rpc-r8a77995"
+- #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 "rpc_regs" and "dirmap"
+- interrupts: interrupt line connected to the RPC SPI controller
+- clock-names: should contain "clk_rpc"
+- clocks: should contain 1 entries for the CPG Module 917 clocks
+
+Example:
+
+	rpc: spi@ee200000 {
+		compatible = "renesas,rpc-r8a77995";
+		reg = <0 0xee200000 0 0x8100>, <0 0x08000000 0 0x4000000>;
+		reg-names = "rpc_regs", "dirmap";
+		clocks = <&cpg CPG_MOD 917>;
+		clock-names = "clk_rpc";
+		#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 2/2] dt-binding: spi: Document Renesas R-Car RPC controller bindings

[Marek Vasut]

On 11/19/2018 11:01 AM, Mason Yang wrote:
> Document the bindings used by the Renesas R-Car D3 RPC controller.
> 
> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> ---
>  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
>  1 file changed, 33 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..8286cc8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> @@ -0,0 +1,33 @@
> +Renesas R-Car D3 RPC controller Device Tree Bindings
> +----------------------------------------------------
> +
> +Required properties:
> +- compatible: should be "renesas,rpc-r8a77995"
> +- #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 "rpc_regs" and "dirmap"
> +- interrupts: interrupt line connected to the RPC SPI controller

Do you also plan to support the RPC HF mode ? And if so, how would that
look in the bindings ? I'd like to avoid having to redesign the bindings
later to handle both the SPI and HF modes.

The HF is roughly a CFI flash with different bus interface.

btw U-Boot has drivers for both the HF and SPI mode:
http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/mtd/renesas_rpc_hf.c
http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/spi/renesas_rpc_spi.c

> +- clock-names: should contain "clk_rpc"
> +- clocks: should contain 1 entries for the CPG Module 917 clocks
> +
> +Example:
> +
> +	rpc: spi@ee200000 {
> +		compatible = "renesas,rpc-r8a77995";
> +		reg = <0 0xee200000 0 0x8100>, <0 0x08000000 0 0x4000000>;
> +		reg-names = "rpc_regs", "dirmap";
> +		clocks = <&cpg CPG_MOD 917>;
> +		clock-names = "clk_rpc";
> +		#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

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

[Boris Brezillon]

On Mon, 19 Nov 2018 14:49:31 +0100
Marek Vasut <marek.vasut@gmail.com> wrote:

> On 11/19/2018 11:01 AM, Mason Yang wrote:
> > Document the bindings used by the Renesas R-Car D3 RPC controller.
> > 
> > Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> > ---
> >  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
> >  1 file changed, 33 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..8286cc8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> > @@ -0,0 +1,33 @@
> > +Renesas R-Car D3 RPC controller Device Tree Bindings
> > +----------------------------------------------------
> > +
> > +Required properties:
> > +- compatible: should be "renesas,rpc-r8a77995"
> > +- #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 "rpc_regs" and "dirmap"
> > +- interrupts: interrupt line connected to the RPC SPI controller  
> 
> Do you also plan to support the RPC HF mode ? And if so, how would that
> look in the bindings ?

Not sure this approach is still accepted, but that's how we solved the
problem for the flexcom block [1].

[1]https://elixir.bootlin.com/linux/v4.20-rc3/source/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt

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

[Marek Vasut]

On 11/19/2018 03:10 PM, Boris Brezillon wrote:
> On Mon, 19 Nov 2018 14:49:31 +0100
> Marek Vasut <marek.vasut@gmail.com> wrote:
> 
>> On 11/19/2018 11:01 AM, Mason Yang wrote:
>>> Document the bindings used by the Renesas R-Car D3 RPC controller.
>>>
>>> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
>>> ---
>>>  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
>>>  1 file changed, 33 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..8286cc8
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>>> @@ -0,0 +1,33 @@
>>> +Renesas R-Car D3 RPC controller Device Tree Bindings
>>> +----------------------------------------------------
>>> +
>>> +Required properties:
>>> +- compatible: should be "renesas,rpc-r8a77995"
>>> +- #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 "rpc_regs" and "dirmap"
>>> +- interrupts: interrupt line connected to the RPC SPI controller  
>>
>> Do you also plan to support the RPC HF mode ? And if so, how would that
>> look in the bindings ?
> 
> Not sure this approach is still accepted, but that's how we solved the
> problem for the flexcom block [1].
> 
> [1]https://elixir.bootlin.com/linux/v4.20-rc3/source/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt

That looks pretty horrible.

In U-Boot we check whether the device hanging under the controller node
is JEDEC SPI flash or CFI flash and based on that decide what the config
of the controller should be (SPI or HF). Not sure that's much better,but
at least it doesn't need extra nodes which do not really represent any
kind of real hardware.

-- 
Best regards,
Marek Vasut

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

[Boris Brezillon]

On Mon, 19 Nov 2018 15:14:07 +0100
Marek Vasut <marek.vasut@gmail.com> wrote:

> On 11/19/2018 03:10 PM, Boris Brezillon wrote:
> > On Mon, 19 Nov 2018 14:49:31 +0100
> > Marek Vasut <marek.vasut@gmail.com> wrote:
> >   
> >> On 11/19/2018 11:01 AM, Mason Yang wrote:  
> >>> Document the bindings used by the Renesas R-Car D3 RPC controller.
> >>>
> >>> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> >>> ---
> >>>  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
> >>>  1 file changed, 33 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..8286cc8
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> >>> @@ -0,0 +1,33 @@
> >>> +Renesas R-Car D3 RPC controller Device Tree Bindings
> >>> +----------------------------------------------------
> >>> +
> >>> +Required properties:
> >>> +- compatible: should be "renesas,rpc-r8a77995"
> >>> +- #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 "rpc_regs" and "dirmap"
> >>> +- interrupts: interrupt line connected to the RPC SPI controller    
> >>
> >> Do you also plan to support the RPC HF mode ? And if so, how would that
> >> look in the bindings ?  
> > 
> > Not sure this approach is still accepted, but that's how we solved the
> > problem for the flexcom block [1].
> > 
> > [1]https://elixir.bootlin.com/linux/v4.20-rc3/source/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt  
> 
> That looks pretty horrible.
> 
> In U-Boot we check whether the device hanging under the controller node
> is JEDEC SPI flash or CFI flash and based on that decide what the config
> of the controller should be (SPI or HF). Not sure that's much better,but
> at least it doesn't need extra nodes which do not really represent any
> kind of real hardware.
> 

The subnodes are not needed, you can just have a property that tells in
which mode the controller is supposed to operate, and the MFD would
create a sub-device that points to the same device_node. Or we can have
a single driver that decides what to declare (a spi_controller or flash
controller), but you'd still have to decide where to place this
driver...

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

[Marek Vasut]

On 11/19/2018 03:43 PM, Boris Brezillon wrote:
> On Mon, 19 Nov 2018 15:14:07 +0100
> Marek Vasut <marek.vasut@gmail.com> wrote:
> 
>> On 11/19/2018 03:10 PM, Boris Brezillon wrote:
>>> On Mon, 19 Nov 2018 14:49:31 +0100
>>> Marek Vasut <marek.vasut@gmail.com> wrote:
>>>   
>>>> On 11/19/2018 11:01 AM, Mason Yang wrote:  
>>>>> Document the bindings used by the Renesas R-Car D3 RPC controller.
>>>>>
>>>>> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
>>>>> ---
>>>>>  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
>>>>>  1 file changed, 33 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..8286cc8
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>>>>> @@ -0,0 +1,33 @@
>>>>> +Renesas R-Car D3 RPC controller Device Tree Bindings
>>>>> +----------------------------------------------------
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible: should be "renesas,rpc-r8a77995"
>>>>> +- #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 "rpc_regs" and "dirmap"
>>>>> +- interrupts: interrupt line connected to the RPC SPI controller    
>>>>
>>>> Do you also plan to support the RPC HF mode ? And if so, how would that
>>>> look in the bindings ?  
>>>
>>> Not sure this approach is still accepted, but that's how we solved the
>>> problem for the flexcom block [1].
>>>
>>> [1]https://elixir.bootlin.com/linux/v4.20-rc3/source/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt  
>>
>> That looks pretty horrible.
>>
>> In U-Boot we check whether the device hanging under the controller node
>> is JEDEC SPI flash or CFI flash and based on that decide what the config
>> of the controller should be (SPI or HF). Not sure that's much better,but
>> at least it doesn't need extra nodes which do not really represent any
>> kind of real hardware.
>>
> 
> The subnodes are not needed, you can just have a property that tells in
> which mode the controller is supposed to operate, and the MFD would
> create a sub-device that points to the same device_node.

Do you even need a dedicated property ? I think you can decide purely on
what node is hanging under the controller (jedec spi nor or cfi nor).

> Or we can have
> a single driver that decides what to declare (a spi_controller or flash
> controller), but you'd still have to decide where to place this
> driver...

I'd definitely prefer a single driver.

-- 
Best regards,
Marek Vasut

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

[Boris Brezillon]

On Mon, 19 Nov 2018 16:12:41 +0100
Marek Vasut <marek.vasut@gmail.com> wrote:

> On 11/19/2018 03:43 PM, Boris Brezillon wrote:
> > On Mon, 19 Nov 2018 15:14:07 +0100
> > Marek Vasut <marek.vasut@gmail.com> wrote:
> >   
> >> On 11/19/2018 03:10 PM, Boris Brezillon wrote:  
> >>> On Mon, 19 Nov 2018 14:49:31 +0100
> >>> Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>     
> >>>> On 11/19/2018 11:01 AM, Mason Yang wrote:    
> >>>>> Document the bindings used by the Renesas R-Car D3 RPC controller.
> >>>>>
> >>>>> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> >>>>> ---
> >>>>>  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
> >>>>>  1 file changed, 33 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..8286cc8
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> >>>>> @@ -0,0 +1,33 @@
> >>>>> +Renesas R-Car D3 RPC controller Device Tree Bindings
> >>>>> +----------------------------------------------------
> >>>>> +
> >>>>> +Required properties:
> >>>>> +- compatible: should be "renesas,rpc-r8a77995"
> >>>>> +- #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 "rpc_regs" and "dirmap"
> >>>>> +- interrupts: interrupt line connected to the RPC SPI controller      
> >>>>
> >>>> Do you also plan to support the RPC HF mode ? And if so, how would that
> >>>> look in the bindings ?    
> >>>
> >>> Not sure this approach is still accepted, but that's how we solved the
> >>> problem for the flexcom block [1].
> >>>
> >>> [1]https://elixir.bootlin.com/linux/v4.20-rc3/source/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt    
> >>
> >> That looks pretty horrible.
> >>
> >> In U-Boot we check whether the device hanging under the controller node
> >> is JEDEC SPI flash or CFI flash and based on that decide what the config
> >> of the controller should be (SPI or HF). Not sure that's much better,but
> >> at least it doesn't need extra nodes which do not really represent any
> >> kind of real hardware.
> >>  
> > 
> > The subnodes are not needed, you can just have a property that tells in
> > which mode the controller is supposed to operate, and the MFD would
> > create a sub-device that points to the same device_node.  
> 
> Do you even need a dedicated property ? I think you can decide purely on
> what node is hanging under the controller (jedec spi nor or cfi nor).

Yes, that could work if they have well-known compatibles. As soon as
people start using flash-specific compats (like some people do for
their SPI NORs) it becomes a maintenance burden.

> 
> > Or we can have
> > a single driver that decides what to declare (a spi_controller or flash
> > controller), but you'd still have to decide where to place this
> > driver...  
> 
> I'd definitely prefer a single driver.
> 

Where would you put this driver? I really don't like the idea of having
MTD drivers spread over the tree. Don't know what's Mark's opinion on
this matter.

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

[Marek Vasut]

On 11/19/2018 04:21 PM, Boris Brezillon wrote:
> On Mon, 19 Nov 2018 16:12:41 +0100
> Marek Vasut <marek.vasut@gmail.com> wrote:
> 
>> On 11/19/2018 03:43 PM, Boris Brezillon wrote:
>>> On Mon, 19 Nov 2018 15:14:07 +0100
>>> Marek Vasut <marek.vasut@gmail.com> wrote:
>>>   
>>>> On 11/19/2018 03:10 PM, Boris Brezillon wrote:  
>>>>> On Mon, 19 Nov 2018 14:49:31 +0100
>>>>> Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>     
>>>>>> On 11/19/2018 11:01 AM, Mason Yang wrote:    
>>>>>>> Document the bindings used by the Renesas R-Car D3 RPC controller.
>>>>>>>
>>>>>>> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
>>>>>>> ---
>>>>>>>  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
>>>>>>>  1 file changed, 33 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..8286cc8
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>>>>>>> @@ -0,0 +1,33 @@
>>>>>>> +Renesas R-Car D3 RPC controller Device Tree Bindings
>>>>>>> +----------------------------------------------------
>>>>>>> +
>>>>>>> +Required properties:
>>>>>>> +- compatible: should be "renesas,rpc-r8a77995"
>>>>>>> +- #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 "rpc_regs" and "dirmap"
>>>>>>> +- interrupts: interrupt line connected to the RPC SPI controller      
>>>>>>
>>>>>> Do you also plan to support the RPC HF mode ? And if so, how would that
>>>>>> look in the bindings ?    
>>>>>
>>>>> Not sure this approach is still accepted, but that's how we solved the
>>>>> problem for the flexcom block [1].
>>>>>
>>>>> [1]https://elixir.bootlin.com/linux/v4.20-rc3/source/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt    
>>>>
>>>> That looks pretty horrible.
>>>>
>>>> In U-Boot we check whether the device hanging under the controller node
>>>> is JEDEC SPI flash or CFI flash and based on that decide what the config
>>>> of the controller should be (SPI or HF). Not sure that's much better,but
>>>> at least it doesn't need extra nodes which do not really represent any
>>>> kind of real hardware.
>>>>  
>>>
>>> The subnodes are not needed, you can just have a property that tells in
>>> which mode the controller is supposed to operate, and the MFD would
>>> create a sub-device that points to the same device_node.  
>>
>> Do you even need a dedicated property ? I think you can decide purely on
>> what node is hanging under the controller (jedec spi nor or cfi nor).
> 
> Yes, that could work if they have well-known compatibles. As soon as
> people start using flash-specific compats (like some people do for
> their SPI NORs) it becomes a maintenance burden.

Which, on this controller, is very likely never gonna happen. Once it
does , we can add a custom property.

>>> Or we can have
>>> a single driver that decides what to declare (a spi_controller or flash
>>> controller), but you'd still have to decide where to place this
>>> driver...  
>>
>> I'd definitely prefer a single driver.
>>
> 
> Where would you put this driver? I really don't like the idea of having
> MTD drivers spread over the tree. Don't know what's Mark's opinion on
> this matter.

Well, it's both CFI (hyperflash) and SF (well, SPI flash) controller, so
where would this go ?

-- 
Best regards,
Marek Vasut

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

[Boris Brezillon]

On Mon, 19 Nov 2018 23:11:31 +0100
Marek Vasut <marek.vasut@gmail.com> wrote:

> On 11/19/2018 04:21 PM, Boris Brezillon wrote:
> > On Mon, 19 Nov 2018 16:12:41 +0100
> > Marek Vasut <marek.vasut@gmail.com> wrote:
> >   
> >> On 11/19/2018 03:43 PM, Boris Brezillon wrote:  
> >>> On Mon, 19 Nov 2018 15:14:07 +0100
> >>> Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>     
> >>>> On 11/19/2018 03:10 PM, Boris Brezillon wrote:    
> >>>>> On Mon, 19 Nov 2018 14:49:31 +0100
> >>>>> Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>       
> >>>>>> On 11/19/2018 11:01 AM, Mason Yang wrote:      
> >>>>>>> Document the bindings used by the Renesas R-Car D3 RPC controller.
> >>>>>>>
> >>>>>>> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> >>>>>>> ---
> >>>>>>>  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
> >>>>>>>  1 file changed, 33 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..8286cc8
> >>>>>>> --- /dev/null
> >>>>>>> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> >>>>>>> @@ -0,0 +1,33 @@
> >>>>>>> +Renesas R-Car D3 RPC controller Device Tree Bindings
> >>>>>>> +----------------------------------------------------
> >>>>>>> +
> >>>>>>> +Required properties:
> >>>>>>> +- compatible: should be "renesas,rpc-r8a77995"
> >>>>>>> +- #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 "rpc_regs" and "dirmap"
> >>>>>>> +- interrupts: interrupt line connected to the RPC SPI controller        
> >>>>>>
> >>>>>> Do you also plan to support the RPC HF mode ? And if so, how would that
> >>>>>> look in the bindings ?      
> >>>>>
> >>>>> Not sure this approach is still accepted, but that's how we solved the
> >>>>> problem for the flexcom block [1].
> >>>>>
> >>>>> [1]https://elixir.bootlin.com/linux/v4.20-rc3/source/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt      
> >>>>
> >>>> That looks pretty horrible.
> >>>>
> >>>> In U-Boot we check whether the device hanging under the controller node
> >>>> is JEDEC SPI flash or CFI flash and based on that decide what the config
> >>>> of the controller should be (SPI or HF). Not sure that's much better,but
> >>>> at least it doesn't need extra nodes which do not really represent any
> >>>> kind of real hardware.
> >>>>    
> >>>
> >>> The subnodes are not needed, you can just have a property that tells in
> >>> which mode the controller is supposed to operate, and the MFD would
> >>> create a sub-device that points to the same device_node.    
> >>
> >> Do you even need a dedicated property ? I think you can decide purely on
> >> what node is hanging under the controller (jedec spi nor or cfi nor).  
> > 
> > Yes, that could work if they have well-known compatibles. As soon as
> > people start using flash-specific compats (like some people do for
> > their SPI NORs) it becomes a maintenance burden.  
> 
> Which, on this controller, is very likely never gonna happen. Once it
> does , we can add a custom property.
> 
> >>> Or we can have
> >>> a single driver that decides what to declare (a spi_controller or flash
> >>> controller), but you'd still have to decide where to place this
> >>> driver...    
> >>
> >> I'd definitely prefer a single driver.
> >>  
> > 
> > Where would you put this driver? I really don't like the idea of having
> > MTD drivers spread over the tree. Don't know what's Mark's opinion on
> > this matter.  
> 
> Well, it's both CFI (hyperflash) and SF (well, SPI flash) controller, so
> where would this go ?
> 

The spi-mem layer is in drivers/spi/ so it could go in drivers/spi/
(spi-mem controller) or drivers/mtd/ (CFI controller). Looks like you
didn't closely follow what has happened in the spi-nor subsystem
lately :P.

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

[Marek Vasut]

On 11/19/2018 11:19 PM, Boris Brezillon wrote:
> On Mon, 19 Nov 2018 23:11:31 +0100
> Marek Vasut <marek.vasut@gmail.com> wrote:
> 
>> On 11/19/2018 04:21 PM, Boris Brezillon wrote:
>>> On Mon, 19 Nov 2018 16:12:41 +0100
>>> Marek Vasut <marek.vasut@gmail.com> wrote:
>>>   
>>>> On 11/19/2018 03:43 PM, Boris Brezillon wrote:  
>>>>> On Mon, 19 Nov 2018 15:14:07 +0100
>>>>> Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>     
>>>>>> On 11/19/2018 03:10 PM, Boris Brezillon wrote:    
>>>>>>> On Mon, 19 Nov 2018 14:49:31 +0100
>>>>>>> Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>       
>>>>>>>> On 11/19/2018 11:01 AM, Mason Yang wrote:      
>>>>>>>>> Document the bindings used by the Renesas R-Car D3 RPC controller.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
>>>>>>>>> ---
>>>>>>>>>  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
>>>>>>>>>  1 file changed, 33 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..8286cc8
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>>>>>>>>> @@ -0,0 +1,33 @@
>>>>>>>>> +Renesas R-Car D3 RPC controller Device Tree Bindings
>>>>>>>>> +----------------------------------------------------
>>>>>>>>> +
>>>>>>>>> +Required properties:
>>>>>>>>> +- compatible: should be "renesas,rpc-r8a77995"
>>>>>>>>> +- #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 "rpc_regs" and "dirmap"
>>>>>>>>> +- interrupts: interrupt line connected to the RPC SPI controller        
>>>>>>>>
>>>>>>>> Do you also plan to support the RPC HF mode ? And if so, how would that
>>>>>>>> look in the bindings ?      
>>>>>>>
>>>>>>> Not sure this approach is still accepted, but that's how we solved the
>>>>>>> problem for the flexcom block [1].
>>>>>>>
>>>>>>> [1]https://elixir.bootlin.com/linux/v4.20-rc3/source/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt      
>>>>>>
>>>>>> That looks pretty horrible.
>>>>>>
>>>>>> In U-Boot we check whether the device hanging under the controller node
>>>>>> is JEDEC SPI flash or CFI flash and based on that decide what the config
>>>>>> of the controller should be (SPI or HF). Not sure that's much better,but
>>>>>> at least it doesn't need extra nodes which do not really represent any
>>>>>> kind of real hardware.
>>>>>>    
>>>>>
>>>>> The subnodes are not needed, you can just have a property that tells in
>>>>> which mode the controller is supposed to operate, and the MFD would
>>>>> create a sub-device that points to the same device_node.    
>>>>
>>>> Do you even need a dedicated property ? I think you can decide purely on
>>>> what node is hanging under the controller (jedec spi nor or cfi nor).  
>>>
>>> Yes, that could work if they have well-known compatibles. As soon as
>>> people start using flash-specific compats (like some people do for
>>> their SPI NORs) it becomes a maintenance burden.  
>>
>> Which, on this controller, is very likely never gonna happen. Once it
>> does , we can add a custom property.
>>
>>>>> Or we can have
>>>>> a single driver that decides what to declare (a spi_controller or flash
>>>>> controller), but you'd still have to decide where to place this
>>>>> driver...    
>>>>
>>>> I'd definitely prefer a single driver.
>>>>  
>>>
>>> Where would you put this driver? I really don't like the idea of having
>>> MTD drivers spread over the tree. Don't know what's Mark's opinion on
>>> this matter.  
>>
>> Well, it's both CFI (hyperflash) and SF (well, SPI flash) controller, so
>> where would this go ?
>>
> 
> The spi-mem layer is in drivers/spi/ so it could go in drivers/spi/
> (spi-mem controller) or drivers/mtd/ (CFI controller).

drivers/mtd is probably a better option, since it's not a generic SPI
controller.


-- 
Best regards,
Marek Vasut

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

[Boris Brezillon]

On Mon, 19 Nov 2018 23:22:45 +0100
Marek Vasut <marek.vasut@gmail.com> wrote:

> On 11/19/2018 11:19 PM, Boris Brezillon wrote:
> > On Mon, 19 Nov 2018 23:11:31 +0100
> > Marek Vasut <marek.vasut@gmail.com> wrote:
> >   
> >> On 11/19/2018 04:21 PM, Boris Brezillon wrote:  
> >>> On Mon, 19 Nov 2018 16:12:41 +0100
> >>> Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>     
> >>>> On 11/19/2018 03:43 PM, Boris Brezillon wrote:    
> >>>>> On Mon, 19 Nov 2018 15:14:07 +0100
> >>>>> Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>       
> >>>>>> On 11/19/2018 03:10 PM, Boris Brezillon wrote:      
> >>>>>>> On Mon, 19 Nov 2018 14:49:31 +0100
> >>>>>>> Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>         
> >>>>>>>> On 11/19/2018 11:01 AM, Mason Yang wrote:        
> >>>>>>>>> Document the bindings used by the Renesas R-Car D3 RPC controller.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> >>>>>>>>> ---
> >>>>>>>>>  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
> >>>>>>>>>  1 file changed, 33 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..8286cc8
> >>>>>>>>> --- /dev/null
> >>>>>>>>> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> >>>>>>>>> @@ -0,0 +1,33 @@
> >>>>>>>>> +Renesas R-Car D3 RPC controller Device Tree Bindings
> >>>>>>>>> +----------------------------------------------------
> >>>>>>>>> +
> >>>>>>>>> +Required properties:
> >>>>>>>>> +- compatible: should be "renesas,rpc-r8a77995"
> >>>>>>>>> +- #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 "rpc_regs" and "dirmap"
> >>>>>>>>> +- interrupts: interrupt line connected to the RPC SPI controller          
> >>>>>>>>
> >>>>>>>> Do you also plan to support the RPC HF mode ? And if so, how would that
> >>>>>>>> look in the bindings ?        
> >>>>>>>
> >>>>>>> Not sure this approach is still accepted, but that's how we solved the
> >>>>>>> problem for the flexcom block [1].
> >>>>>>>
> >>>>>>> [1]https://elixir.bootlin.com/linux/v4.20-rc3/source/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt        
> >>>>>>
> >>>>>> That looks pretty horrible.
> >>>>>>
> >>>>>> In U-Boot we check whether the device hanging under the controller node
> >>>>>> is JEDEC SPI flash or CFI flash and based on that decide what the config
> >>>>>> of the controller should be (SPI or HF). Not sure that's much better,but
> >>>>>> at least it doesn't need extra nodes which do not really represent any
> >>>>>> kind of real hardware.
> >>>>>>      
> >>>>>
> >>>>> The subnodes are not needed, you can just have a property that tells in
> >>>>> which mode the controller is supposed to operate, and the MFD would
> >>>>> create a sub-device that points to the same device_node.      
> >>>>
> >>>> Do you even need a dedicated property ? I think you can decide purely on
> >>>> what node is hanging under the controller (jedec spi nor or cfi nor).    
> >>>
> >>> Yes, that could work if they have well-known compatibles. As soon as
> >>> people start using flash-specific compats (like some people do for
> >>> their SPI NORs) it becomes a maintenance burden.    
> >>
> >> Which, on this controller, is very likely never gonna happen. Once it
> >> does , we can add a custom property.
> >>  
> >>>>> Or we can have
> >>>>> a single driver that decides what to declare (a spi_controller or flash
> >>>>> controller), but you'd still have to decide where to place this
> >>>>> driver...      
> >>>>
> >>>> I'd definitely prefer a single driver.
> >>>>    
> >>>
> >>> Where would you put this driver? I really don't like the idea of having
> >>> MTD drivers spread over the tree. Don't know what's Mark's opinion on
> >>> this matter.    
> >>
> >> Well, it's both CFI (hyperflash) and SF (well, SPI flash) controller, so
> >> where would this go ?
> >>  
> > 
> > The spi-mem layer is in drivers/spi/ so it could go in drivers/spi/
> > (spi-mem controller) or drivers/mtd/ (CFI controller).  
> 
> drivers/mtd is probably a better option, since it's not a generic SPI
> controller.
> 

No, spi-mem controller drivers should go in drivers/spi/ even if they
don't implement the generic SPI interface (it's allowed to only
implement the spi_mem interface).

> 

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

[Marek Vasut]

On 11/19/2018 11:25 PM, Boris Brezillon wrote:
> On Mon, 19 Nov 2018 23:22:45 +0100
> Marek Vasut <marek.vasut@gmail.com> wrote:
> 
>> On 11/19/2018 11:19 PM, Boris Brezillon wrote:
>>> On Mon, 19 Nov 2018 23:11:31 +0100
>>> Marek Vasut <marek.vasut@gmail.com> wrote:
>>>   
>>>> On 11/19/2018 04:21 PM, Boris Brezillon wrote:  
>>>>> On Mon, 19 Nov 2018 16:12:41 +0100
>>>>> Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>     
>>>>>> On 11/19/2018 03:43 PM, Boris Brezillon wrote:    
>>>>>>> On Mon, 19 Nov 2018 15:14:07 +0100
>>>>>>> Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>       
>>>>>>>> On 11/19/2018 03:10 PM, Boris Brezillon wrote:      
>>>>>>>>> On Mon, 19 Nov 2018 14:49:31 +0100
>>>>>>>>> Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>>>         
>>>>>>>>>> On 11/19/2018 11:01 AM, Mason Yang wrote:        
>>>>>>>>>>> Document the bindings used by the Renesas R-Car D3 RPC controller.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
>>>>>>>>>>> ---
>>>>>>>>>>>  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
>>>>>>>>>>>  1 file changed, 33 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..8286cc8
>>>>>>>>>>> --- /dev/null
>>>>>>>>>>> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>>>>>>>>>>> @@ -0,0 +1,33 @@
>>>>>>>>>>> +Renesas R-Car D3 RPC controller Device Tree Bindings
>>>>>>>>>>> +----------------------------------------------------
>>>>>>>>>>> +
>>>>>>>>>>> +Required properties:
>>>>>>>>>>> +- compatible: should be "renesas,rpc-r8a77995"
>>>>>>>>>>> +- #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 "rpc_regs" and "dirmap"
>>>>>>>>>>> +- interrupts: interrupt line connected to the RPC SPI controller          
>>>>>>>>>>
>>>>>>>>>> Do you also plan to support the RPC HF mode ? And if so, how would that
>>>>>>>>>> look in the bindings ?        
>>>>>>>>>
>>>>>>>>> Not sure this approach is still accepted, but that's how we solved the
>>>>>>>>> problem for the flexcom block [1].
>>>>>>>>>
>>>>>>>>> [1]https://elixir.bootlin.com/linux/v4.20-rc3/source/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt        
>>>>>>>>
>>>>>>>> That looks pretty horrible.
>>>>>>>>
>>>>>>>> In U-Boot we check whether the device hanging under the controller node
>>>>>>>> is JEDEC SPI flash or CFI flash and based on that decide what the config
>>>>>>>> of the controller should be (SPI or HF). Not sure that's much better,but
>>>>>>>> at least it doesn't need extra nodes which do not really represent any
>>>>>>>> kind of real hardware.
>>>>>>>>      
>>>>>>>
>>>>>>> The subnodes are not needed, you can just have a property that tells in
>>>>>>> which mode the controller is supposed to operate, and the MFD would
>>>>>>> create a sub-device that points to the same device_node.      
>>>>>>
>>>>>> Do you even need a dedicated property ? I think you can decide purely on
>>>>>> what node is hanging under the controller (jedec spi nor or cfi nor).    
>>>>>
>>>>> Yes, that could work if they have well-known compatibles. As soon as
>>>>> people start using flash-specific compats (like some people do for
>>>>> their SPI NORs) it becomes a maintenance burden.    
>>>>
>>>> Which, on this controller, is very likely never gonna happen. Once it
>>>> does , we can add a custom property.
>>>>  
>>>>>>> Or we can have
>>>>>>> a single driver that decides what to declare (a spi_controller or flash
>>>>>>> controller), but you'd still have to decide where to place this
>>>>>>> driver...      
>>>>>>
>>>>>> I'd definitely prefer a single driver.
>>>>>>    
>>>>>
>>>>> Where would you put this driver? I really don't like the idea of having
>>>>> MTD drivers spread over the tree. Don't know what's Mark's opinion on
>>>>> this matter.    
>>>>
>>>> Well, it's both CFI (hyperflash) and SF (well, SPI flash) controller, so
>>>> where would this go ?
>>>>  
>>>
>>> The spi-mem layer is in drivers/spi/ so it could go in drivers/spi/
>>> (spi-mem controller) or drivers/mtd/ (CFI controller).  
>>
>> drivers/mtd is probably a better option, since it's not a generic SPI
>> controller.
>>
> 
> No, spi-mem controller drivers should go in drivers/spi/ even if they
> don't implement the generic SPI interface (it's allowed to only
> implement the spi_mem interface).

Except this is not only SPI MEM controller, this is also hyperflash
(that is, CFI) controller. It can drive both types of chips. Thus , I
think it fits better in drivers/mtd/ .

-- 
Best regards,
Marek Vasut

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

[Boris Brezillon]

On Mon, 19 Nov 2018 23:29:00 +0100
Marek Vasut <marek.vasut@gmail.com> wrote:

> On 11/19/2018 11:25 PM, Boris Brezillon wrote:
> > On Mon, 19 Nov 2018 23:22:45 +0100
> > Marek Vasut <marek.vasut@gmail.com> wrote:
> >   
> >> On 11/19/2018 11:19 PM, Boris Brezillon wrote:  
> >>> On Mon, 19 Nov 2018 23:11:31 +0100
> >>> Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>     
> >>>> On 11/19/2018 04:21 PM, Boris Brezillon wrote:    
> >>>>> On Mon, 19 Nov 2018 16:12:41 +0100
> >>>>> Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>       
> >>>>>> On 11/19/2018 03:43 PM, Boris Brezillon wrote:      
> >>>>>>> On Mon, 19 Nov 2018 15:14:07 +0100
> >>>>>>> Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>         
> >>>>>>>> On 11/19/2018 03:10 PM, Boris Brezillon wrote:        
> >>>>>>>>> On Mon, 19 Nov 2018 14:49:31 +0100
> >>>>>>>>> Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>>>           
> >>>>>>>>>> On 11/19/2018 11:01 AM, Mason Yang wrote:          
> >>>>>>>>>>> Document the bindings used by the Renesas R-Car D3 RPC controller.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> >>>>>>>>>>> ---
> >>>>>>>>>>>  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
> >>>>>>>>>>>  1 file changed, 33 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..8286cc8
> >>>>>>>>>>> --- /dev/null
> >>>>>>>>>>> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> >>>>>>>>>>> @@ -0,0 +1,33 @@
> >>>>>>>>>>> +Renesas R-Car D3 RPC controller Device Tree Bindings
> >>>>>>>>>>> +----------------------------------------------------
> >>>>>>>>>>> +
> >>>>>>>>>>> +Required properties:
> >>>>>>>>>>> +- compatible: should be "renesas,rpc-r8a77995"
> >>>>>>>>>>> +- #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 "rpc_regs" and "dirmap"
> >>>>>>>>>>> +- interrupts: interrupt line connected to the RPC SPI controller            
> >>>>>>>>>>
> >>>>>>>>>> Do you also plan to support the RPC HF mode ? And if so, how would that
> >>>>>>>>>> look in the bindings ?          
> >>>>>>>>>
> >>>>>>>>> Not sure this approach is still accepted, but that's how we solved the
> >>>>>>>>> problem for the flexcom block [1].
> >>>>>>>>>
> >>>>>>>>> [1]https://elixir.bootlin.com/linux/v4.20-rc3/source/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt          
> >>>>>>>>
> >>>>>>>> That looks pretty horrible.
> >>>>>>>>
> >>>>>>>> In U-Boot we check whether the device hanging under the controller node
> >>>>>>>> is JEDEC SPI flash or CFI flash and based on that decide what the config
> >>>>>>>> of the controller should be (SPI or HF). Not sure that's much better,but
> >>>>>>>> at least it doesn't need extra nodes which do not really represent any
> >>>>>>>> kind of real hardware.
> >>>>>>>>        
> >>>>>>>
> >>>>>>> The subnodes are not needed, you can just have a property that tells in
> >>>>>>> which mode the controller is supposed to operate, and the MFD would
> >>>>>>> create a sub-device that points to the same device_node.        
> >>>>>>
> >>>>>> Do you even need a dedicated property ? I think you can decide purely on
> >>>>>> what node is hanging under the controller (jedec spi nor or cfi nor).      
> >>>>>
> >>>>> Yes, that could work if they have well-known compatibles. As soon as
> >>>>> people start using flash-specific compats (like some people do for
> >>>>> their SPI NORs) it becomes a maintenance burden.      
> >>>>
> >>>> Which, on this controller, is very likely never gonna happen. Once it
> >>>> does , we can add a custom property.
> >>>>    
> >>>>>>> Or we can have
> >>>>>>> a single driver that decides what to declare (a spi_controller or flash
> >>>>>>> controller), but you'd still have to decide where to place this
> >>>>>>> driver...        
> >>>>>>
> >>>>>> I'd definitely prefer a single driver.
> >>>>>>      
> >>>>>
> >>>>> Where would you put this driver? I really don't like the idea of having
> >>>>> MTD drivers spread over the tree. Don't know what's Mark's opinion on
> >>>>> this matter.      
> >>>>
> >>>> Well, it's both CFI (hyperflash) and SF (well, SPI flash) controller, so
> >>>> where would this go ?
> >>>>    
> >>>
> >>> The spi-mem layer is in drivers/spi/ so it could go in drivers/spi/
> >>> (spi-mem controller) or drivers/mtd/ (CFI controller).    
> >>
> >> drivers/mtd is probably a better option, since it's not a generic SPI
> >> controller.
> >>  
> > 
> > No, spi-mem controller drivers should go in drivers/spi/ even if they
> > don't implement the generic SPI interface (it's allowed to only
> > implement the spi_mem interface).  
> 
> Except this is not only SPI MEM controller, this is also hyperflash
> (that is, CFI) controller. It can drive both types of chips. Thus , I
> think it fits better in drivers/mtd/ .
> 

Okay, then I guess we need an ack from Mark on that.

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

[masonccyang]

[-- Attachment #1: Type: text/plain, Size: 3986 bytes --]

Hi Marek, 

Marek Vasut <marek.vasut@gmail.com> 已在 2018/11/19 下午 09:49:31 上寫入: 

> Marek Vasut <marek.vasut@gmail.com> 
> 2018/11/19 下午 09:49
> 
> To
> 
> Mason Yang <masonccyang@mxic.com.tw>, broonie@kernel.org, 
> tpiepho@impinj.com, linux-kernel@vger.kernel.org, linux-
> spi@vger.kernel.org, linux-renesas-soc@vger.kernel.org, Simon Horman
> <horms@verge.net.au>, 
> 
> cc
> 
> boris.brezillon@bootlin.com, juliensu@mxic.com.tw, Geert 
> Uytterhoeven <geert+renesas@glider.be>, zhengxunli@mxic.com.tw
> 
> Subject
> 
> Re: [PATCH 2/2] dt-binding: spi: Document Renesas R-Car RPC 
> controller bindings
> 
> On 11/19/2018 11:01 AM, Mason Yang wrote:
> > Document the bindings used by the Renesas R-Car D3 RPC controller.
> > 
> > Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> > ---
> >  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 +++++++++
> +++++++++++++
> >  1 file changed, 33 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..8286cc8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> > @@ -0,0 +1,33 @@
> > +Renesas R-Car D3 RPC controller Device Tree Bindings
> > +----------------------------------------------------
> > +
> > +Required properties:
> > +- compatible: should be "renesas,rpc-r8a77995"
> > +- #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 "rpc_regs" and "dirmap"
> > +- interrupts: interrupt line connected to the RPC SPI controller
> 
> Do you also plan to support the RPC HF mode ? And if so, how would that
> look in the bindings ? I'd like to avoid having to redesign the bindings
> later to handle both the SPI and HF modes.
> 

I patched this RPC SPI/Octa driver is for mx25uw51245g and you may also 
refer to Boris's patch. [1][2][3]

[1] https://patchwork.kernel.org/cover/10638055/
[2] https://patchwork.kernel.org/patch/10638057/
[3] https://patchwork.kernel.org/patch/10638085/

thanks & best regards,
Mason

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

[-- Attachment #2: Type: text/html, Size: 4554 bytes --]

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

[Marek Vasut]

On 11/20/2018 06:42 AM, masonccyang@mxic.com.tw wrote:
> Hi Marek,
> 
> Marek Vasut <marek.vasut@gmail.com> 已在 2018/11/19 下午 09:49:31 上寫入:
> 
>> Marek Vasut <marek.vasut@gmail.com>
>> 2018/11/19 下午 09:49
>>
>> To
>>
>> Mason Yang <masonccyang@mxic.com.tw>, broonie@kernel.org,
>> tpiepho@impinj.com, linux-kernel@vger.kernel.org, linux-
>> spi@vger.kernel.org, linux-renesas-soc@vger.kernel.org, Simon Horman
>> <horms@verge.net.au>,
>>
>> cc
>>
>> boris.brezillon@bootlin.com, juliensu@mxic.com.tw, Geert
>> Uytterhoeven <geert+renesas@glider.be>, zhengxunli@mxic.com.tw
>>
>> Subject
>>
>> Re: [PATCH 2/2] dt-binding: spi: Document Renesas R-Car RPC
>> controller bindings
>>
>> On 11/19/2018 11:01 AM, Mason Yang wrote:
>> > Document the bindings used by the Renesas R-Car D3 RPC controller.
>> >
>> > Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
>> > ---
>> >  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 +++++++++
>> +++++++++++++
>> >  1 file changed, 33 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..8286cc8
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>> > @@ -0,0 +1,33 @@
>> > +Renesas R-Car D3 RPC controller Device Tree Bindings
>> > +----------------------------------------------------
>> > +
>> > +Required properties:
>> > +- compatible: should be "renesas,rpc-r8a77995"
>> > +- #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 "rpc_regs" and "dirmap"
>> > +- interrupts: interrupt line connected to the RPC SPI controller
>>
>> Do you also plan to support the RPC HF mode ? And if so, how would that
>> look in the bindings ? I'd like to avoid having to redesign the bindings
>> later to handle both the SPI and HF modes.
>>
> 
> I patched this RPC SPI/Octa driver is for mx25uw51245g and you may also
> refer to Boris's patch. [1][2][3]

Does this mean the driver is specific to one particular kind of flash ?

> [1] https://patchwork.kernel.org/cover/10638055/
> [2] https://patchwork.kernel.org/patch/10638057/
> [3] https://patchwork.kernel.org/patch/10638085/


-- 
Best regards,
Marek Vasut

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

[masonccyang]

[-- Attachment #1: Type: text/plain, Size: 3355 bytes --]

Hi Marek,

> Marek Vasut <marek.vasut@gmail.com> 
> 2018/11/20 下午 08:57
> 
> >> > 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..8286cc8
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> >> > @@ -0,0 +1,33 @@
> >> > +Renesas R-Car D3 RPC controller Device Tree Bindings
> >> > +----------------------------------------------------
> >> > +
> >> > +Required properties:
> >> > +- compatible: should be "renesas,rpc-r8a77995"
> >> > +- #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 "rpc_regs" and "dirmap"
> >> > +- interrupts: interrupt line connected to the RPC SPI controller
> >>
> >> Do you also plan to support the RPC HF mode ? And if so, how would 
that
> >> look in the bindings ? I'd like to avoid having to redesign the 
bindings
> >> later to handle both the SPI and HF modes.
> >>
> > 
> > I patched this RPC SPI/Octa driver is for mx25uw51245g and you may 
also
> > refer to Boris's patch. [1][2][3]
> 
> Does this mean the driver is specific to one particular kind of flash ?
> 

No, this driver supports all SPI flash, spi, qspi and octa flash.

The target is Octa 8-8-8 DDR2 mode once spi-nor layer is merged.[1][2][3]

thanks & best regards,
Mason

> > [1] https://patchwork.kernel.org/cover/10638055/
> > [2] https://patchwork.kernel.org/patch/10638057/
> > [3] https://patchwork.kernel.org/patch/10638085/
> 
> 



CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

[-- Attachment #2: Type: text/html, Size: 3703 bytes --]

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

[Marek Vasut]

On 11/21/2018 01:53 AM, masonccyang@mxic.com.tw wrote:
> Hi Marek,

Hi,

>> Marek Vasut <marek.vasut@gmail.com>
>> 2018/11/20 下午 08:57
>>
>> >> > 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..8286cc8
>> >> > --- /dev/null
>> >> > +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>> >> > @@ -0,0 +1,33 @@
>> >> > +Renesas R-Car D3 RPC controller Device Tree Bindings
>> >> > +----------------------------------------------------
>> >> > +
>> >> > +Required properties:
>> >> > +- compatible: should be "renesas,rpc-r8a77995"
>> >> > +- #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 "rpc_regs" and "dirmap"
>> >> > +- interrupts: interrupt line connected to the RPC SPI controller
>> >>
>> >> Do you also plan to support the RPC HF mode ? And if so, how would that
>> >> look in the bindings ? I'd like to avoid having to redesign the
> bindings
>> >> later to handle both the SPI and HF modes.
>> >>
>> >
>> > I patched this RPC SPI/Octa driver is for mx25uw51245g and you may also
>> > refer to Boris's patch. [1][2][3]
>>
>> Does this mean the driver is specific to one particular kind of flash ?
>>
> 
> No, this driver supports all SPI flash, spi, qspi and octa flash.
> 
> The target is Octa 8-8-8 DDR2 mode once spi-nor layer is merged.[1][2][3]

The HyperFlash must also be supported, cfr my previous comment that I'd
like to avoid redesigning the bindings again when the HF mode is added.

-- 
Best regards,
Marek Vasut

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

[Geert Uytterhoeven]

Hi Mason,

On Mon, Nov 19, 2018 at 11:02 AM Mason Yang <masonccyang@mxic.com.tw> wrote:
> Document the bindings used by the Renesas R-Car D3 RPC controller.
>
> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>

Thanks for your patch

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> @@ -0,0 +1,33 @@
> +Renesas R-Car D3 RPC controller Device Tree Bindings

R-Car Gen3

> +----------------------------------------------------
> +
> +Required properties:
> +- compatible: should be "renesas,rpc-r8a77995"

Please use "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 "rpc_regs" and "dirmap"
> +- interrupts: interrupt line connected to the RPC SPI controller
> +- clock-names: should contain "clk_rpc"
> +- clocks: should contain 1 entries for the CPG Module 917 clocks

... for the module's clock

> +
> +Example:
> +
> +       rpc: spi@ee200000 {
> +               compatible = "renesas,rpc-r8a77995";
> +               reg = <0 0xee200000 0 0x8100>, <0 0x08000000 0 0x4000000>;
> +               reg-names = "rpc_regs", "dirmap";
> +               clocks = <&cpg CPG_MOD 917>;
> +               clock-names = "clk_rpc";
> +               #address-cells = <1>;
> +               #size-cells = <0>;

power-domains?
resets?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 25555 bytes --]

Hi Mason,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on spi/for-next]
[also build test ERROR on v4.20-rc3 next-20181120]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Mason-Yang/spi-Add-Renesas-R-Car-RPC-SPI-controller-driver/20181120-020310
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

>> drivers/spi/spi-renesas-rpc.c:283:54: warning: incorrect type in argument 2 (different address spaces)
   drivers/spi/spi-renesas-rpc.c:283:54:    expected void const volatile [noderef] <asn:2>*addr
   drivers/spi/spi-renesas-rpc.c:283:54:    got void *<noident>
>> drivers/spi/spi-renesas-rpc.c:369:31: error: using member 'mem' in incomplete struct spi_mem_dirmap_desc
>> drivers/spi/spi-renesas-rpc.c:372:13: error: using member 'info' in incomplete struct spi_mem_dirmap_desc
   drivers/spi/spi-renesas-rpc.c:375:41: error: using member 'mem' in incomplete struct spi_mem_dirmap_desc
   drivers/spi/spi-renesas-rpc.c:379:41: error: using member 'mem' in incomplete struct spi_mem_dirmap_desc
   drivers/spi/spi-renesas-rpc.c:392:50: error: using member 'info' in incomplete struct spi_mem_dirmap_desc
   drivers/spi/spi-renesas-rpc.c:400:31: error: using member 'mem' in incomplete struct spi_mem_dirmap_desc
   drivers/spi/spi-renesas-rpc.c:403:13: error: using member 'info' in incomplete struct spi_mem_dirmap_desc
   drivers/spi/spi-renesas-rpc.c:409:41: error: using member 'mem' in incomplete struct spi_mem_dirmap_desc
   drivers/spi/spi-renesas-rpc.c:413:41: error: using member 'mem' in incomplete struct spi_mem_dirmap_desc
   drivers/spi/spi-renesas-rpc.c:445:31: error: using member 'mem' in incomplete struct spi_mem_dirmap_desc
   drivers/spi/spi-renesas-rpc.c:447:17: error: using member 'info' in incomplete struct spi_mem_dirmap_desc
   drivers/spi/spi-renesas-rpc.c:447:37: error: using member 'info' in incomplete struct spi_mem_dirmap_desc
   drivers/spi/spi-renesas-rpc.c:450:42: error: using member 'mem' in incomplete struct spi_mem_dirmap_desc
   drivers/spi/spi-renesas-rpc.c:454:17: error: using member 'info' in incomplete struct spi_mem_dirmap_desc
>> drivers/spi/spi-renesas-rpc.c:484:10: error: unknown field name in initializer
   drivers/spi/spi-renesas-rpc.c:485:10: error: unknown field name in initializer
   drivers/spi/spi-renesas-rpc.c:486:10: error: unknown field name in initializer
>> drivers/spi/spi-renesas-rpc.c:372:13: warning: unknown expression (8 46)
   drivers/spi/spi-renesas-rpc.c:403:13: warning: unknown expression (8 46)
   drivers/spi/spi-renesas-rpc.c:447:23: warning: unknown expression (8 46)
   drivers/spi/spi-renesas-rpc.c:447:43: warning: unknown expression (8 46)
   drivers/spi/spi-renesas-rpc.c:454:36: warning: unknown expression (8 46)
   drivers/spi/spi-renesas-rpc.c:366:47: warning: 'struct spi_mem_dirmap_desc' declared inside parameter list will not be visible outside of this definition or declaration
    static ssize_t rpc_spi_mem_dirmap_read(struct spi_mem_dirmap_desc *desc,
                                                  ^~~~~~~~~~~~~~~~~~~
   In file included from drivers/spi/spi-renesas-rpc.c:18:0:
   drivers/spi/spi-renesas-rpc.c: In function 'rpc_spi_mem_dirmap_read':
   drivers/spi/spi-renesas-rpc.c:369:51: error: dereferencing pointer to incomplete type 'struct spi_mem_dirmap_desc'
     struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
                                                      ^
   include/linux/spi/spi.h:1333:66: note: in definition of macro 'spi_master_get_devdata'
    #define spi_master_get_devdata(_ctlr) spi_controller_get_devdata(_ctlr)
                                                                     ^~~~~
   drivers/spi/spi-renesas-rpc.c: At top level:
   drivers/spi/spi-renesas-rpc.c:397:48: warning: 'struct spi_mem_dirmap_desc' declared inside parameter list will not be visible outside of this definition or declaration
    static ssize_t rpc_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
                                                   ^~~~~~~~~~~~~~~~~~~
   In file included from drivers/spi/spi-renesas-rpc.c:18:0:
   drivers/spi/spi-renesas-rpc.c: In function 'rpc_spi_mem_dirmap_write':
   drivers/spi/spi-renesas-rpc.c:400:51: error: dereferencing pointer to incomplete type 'struct spi_mem_dirmap_desc'
     struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
                                                      ^
   include/linux/spi/spi.h:1333:66: note: in definition of macro 'spi_master_get_devdata'
    #define spi_master_get_devdata(_ctlr) spi_controller_get_devdata(_ctlr)
                                                                     ^~~~~
   drivers/spi/spi-renesas-rpc.c: At top level:
   drivers/spi/spi-renesas-rpc.c:443:45: warning: 'struct spi_mem_dirmap_desc' declared inside parameter list will not be visible outside of this definition or declaration
    static int rpc_spi_mem_dirmap_create(struct spi_mem_dirmap_desc *desc)
                                                ^~~~~~~~~~~~~~~~~~~
   In file included from drivers/spi/spi-renesas-rpc.c:18:0:
   drivers/spi/spi-renesas-rpc.c: In function 'rpc_spi_mem_dirmap_create':
   drivers/spi/spi-renesas-rpc.c:445:51: error: dereferencing pointer to incomplete type 'struct spi_mem_dirmap_desc'
     struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
                                                      ^
   include/linux/spi/spi.h:1333:66: note: in definition of macro 'spi_master_get_devdata'
    #define spi_master_get_devdata(_ctlr) spi_controller_get_devdata(_ctlr)
                                                                     ^~~~~
   drivers/spi/spi-renesas-rpc.c: At top level:
   drivers/spi/spi-renesas-rpc.c:484:3: error: 'const struct spi_controller_mem_ops' has no member named 'dirmap_create'
     .dirmap_create = rpc_spi_mem_dirmap_create,
      ^~~~~~~~~~~~~
   drivers/spi/spi-renesas-rpc.c:484:19: error: positional initialization of field in 'struct' declared with 'designated_init' attribute [-Werror=designated-init]
     .dirmap_create = rpc_spi_mem_dirmap_create,
                      ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/spi/spi-renesas-rpc.c:484:19: note: (near initialization for 'rpc_spi_mem_ops')
   drivers/spi/spi-renesas-rpc.c:484:19: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
   drivers/spi/spi-renesas-rpc.c:484:19: note: (near initialization for 'rpc_spi_mem_ops.supports_op')
   drivers/spi/spi-renesas-rpc.c:485:3: error: 'const struct spi_controller_mem_ops' has no member named 'dirmap_read'
     .dirmap_read = rpc_spi_mem_dirmap_read,
      ^~~~~~~~~~~
   drivers/spi/spi-renesas-rpc.c:485:17: error: positional initialization of field in 'struct' declared with 'designated_init' attribute [-Werror=designated-init]
     .dirmap_read = rpc_spi_mem_dirmap_read,
                    ^~~~~~~~~~~~~~~~~~~~~~~
   drivers/spi/spi-renesas-rpc.c:485:17: note: (near initialization for 'rpc_spi_mem_ops')
   drivers/spi/spi-renesas-rpc.c:485:17: warning: excess elements in struct initializer
   drivers/spi/spi-renesas-rpc.c:485:17: note: (near initialization for 'rpc_spi_mem_ops')
   drivers/spi/spi-renesas-rpc.c:486:3: error: 'const struct spi_controller_mem_ops' has no member named 'dirmap_write'
     .dirmap_write = rpc_spi_mem_dirmap_write,
      ^~~~~~~~~~~~
   drivers/spi/spi-renesas-rpc.c:486:18: error: positional initialization of field in 'struct' declared with 'designated_init' attribute [-Werror=designated-init]
     .dirmap_write = rpc_spi_mem_dirmap_write,
                     ^~~~~~~~~~~~~~~~~~~~~~~~
   drivers/spi/spi-renesas-rpc.c:486:18: note: (near initialization for 'rpc_spi_mem_ops')
   drivers/spi/spi-renesas-rpc.c:486:18: warning: excess elements in struct initializer
   drivers/spi/spi-renesas-rpc.c:486:18: note: (near initialization for 'rpc_spi_mem_ops')
   cc1: some warnings being treated as errors

vim +/mem +369 drivers/spi/spi-renesas-rpc.c

c4789a83 Mason Yang 2018-11-19  226  
c4789a83 Mason Yang 2018-11-19  227  static int rpc_spi_io_xfer(struct rpc_spi *rpc,
c4789a83 Mason Yang 2018-11-19  228  			   const void *tx_buf, void *rx_buf)
c4789a83 Mason Yang 2018-11-19  229  {
c4789a83 Mason Yang 2018-11-19  230  	u32 smenr, smcr, data, pos = 0;
c4789a83 Mason Yang 2018-11-19  231  	int ret = 0;
c4789a83 Mason Yang 2018-11-19  232  
c4789a83 Mason Yang 2018-11-19  233  	writel(RPC_CMNCR_MD | RPC_CMNCR_SFDE | RPC_CMNCR_MOIIO_HIZ |
c4789a83 Mason Yang 2018-11-19  234  	       RPC_CMNCR_IOFV_HIZ | RPC_CMNCR_BSZ(0), rpc->regs + RPC_CMNCR);
c4789a83 Mason Yang 2018-11-19  235  	writel(0x0, rpc->regs + RPC_SMDRENR);
c4789a83 Mason Yang 2018-11-19  236  
c4789a83 Mason Yang 2018-11-19  237  	if (tx_buf) {
c4789a83 Mason Yang 2018-11-19  238  		writel(rpc->cmd, rpc->regs + RPC_SMCMR);
c4789a83 Mason Yang 2018-11-19  239  		writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
c4789a83 Mason Yang 2018-11-19  240  		writel(rpc->addr, rpc->regs + RPC_SMADR);
c4789a83 Mason Yang 2018-11-19  241  		smenr = rpc->smenr;
c4789a83 Mason Yang 2018-11-19  242  
c4789a83 Mason Yang 2018-11-19  243  		while (pos < rpc->xferlen) {
c4789a83 Mason Yang 2018-11-19  244  			u32 nbytes = rpc->xferlen  - pos;
c4789a83 Mason Yang 2018-11-19  245  
c4789a83 Mason Yang 2018-11-19  246  			writel(*(u32 *)(tx_buf + pos), rpc->regs + RPC_SMWDR0);
c4789a83 Mason Yang 2018-11-19  247  
c4789a83 Mason Yang 2018-11-19  248  			if (nbytes > 4) {
c4789a83 Mason Yang 2018-11-19  249  				nbytes = 4;
c4789a83 Mason Yang 2018-11-19  250  				smcr = rpc->smcr |
c4789a83 Mason Yang 2018-11-19  251  				       RPC_SMCR_SPIE | RPC_SMCR_SSLKP;
c4789a83 Mason Yang 2018-11-19  252  			} else {
c4789a83 Mason Yang 2018-11-19  253  				smcr = rpc->smcr | RPC_SMCR_SPIE;
c4789a83 Mason Yang 2018-11-19  254  			}
c4789a83 Mason Yang 2018-11-19  255  
c4789a83 Mason Yang 2018-11-19  256  			writel(smenr, rpc->regs + RPC_SMENR);
c4789a83 Mason Yang 2018-11-19  257  			writel(smcr, rpc->regs + RPC_SMCR);
c4789a83 Mason Yang 2018-11-19  258  			ret = wait_msg_xfer_end(rpc);
c4789a83 Mason Yang 2018-11-19  259  			if (ret)
c4789a83 Mason Yang 2018-11-19  260  				goto out;
c4789a83 Mason Yang 2018-11-19  261  
c4789a83 Mason Yang 2018-11-19  262  			pos += nbytes;
c4789a83 Mason Yang 2018-11-19  263  			smenr = rpc->smenr & ~RPC_SMENR_CDE &
c4789a83 Mason Yang 2018-11-19  264  					     ~RPC_SMENR_ADE(0xf);
c4789a83 Mason Yang 2018-11-19  265  		}
c4789a83 Mason Yang 2018-11-19  266  	} else if (rx_buf) {
c4789a83 Mason Yang 2018-11-19  267  		while (pos < rpc->xferlen) {
c4789a83 Mason Yang 2018-11-19  268  			u32 nbytes = rpc->xferlen  - pos;
c4789a83 Mason Yang 2018-11-19  269  
c4789a83 Mason Yang 2018-11-19  270  			if (nbytes > 4)
c4789a83 Mason Yang 2018-11-19  271  				nbytes = 4;
c4789a83 Mason Yang 2018-11-19  272  
c4789a83 Mason Yang 2018-11-19  273  			writel(rpc->cmd, rpc->regs + RPC_SMCMR);
c4789a83 Mason Yang 2018-11-19  274  			writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
c4789a83 Mason Yang 2018-11-19  275  			writel(rpc->addr + pos, rpc->regs + RPC_SMADR);
c4789a83 Mason Yang 2018-11-19  276  			writel(rpc->smenr, rpc->regs + RPC_SMENR);
c4789a83 Mason Yang 2018-11-19  277  			writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
c4789a83 Mason Yang 2018-11-19  278  			ret = wait_msg_xfer_end(rpc);
c4789a83 Mason Yang 2018-11-19  279  			if (ret)
c4789a83 Mason Yang 2018-11-19  280  				goto out;
c4789a83 Mason Yang 2018-11-19  281  
c4789a83 Mason Yang 2018-11-19  282  			data = readl(rpc->regs + RPC_SMRDR0);
c4789a83 Mason Yang 2018-11-19 @283  			memcpy_fromio(rx_buf + pos, (void *)&data, nbytes);
c4789a83 Mason Yang 2018-11-19  284  			pos += nbytes;
c4789a83 Mason Yang 2018-11-19  285  		}
c4789a83 Mason Yang 2018-11-19  286  	} else {
c4789a83 Mason Yang 2018-11-19  287  		writel(rpc->cmd, rpc->regs + RPC_SMCMR);
c4789a83 Mason Yang 2018-11-19  288  		writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
c4789a83 Mason Yang 2018-11-19  289  		writel(rpc->addr + pos, rpc->regs + RPC_SMADR);
c4789a83 Mason Yang 2018-11-19  290  		writel(rpc->smenr, rpc->regs + RPC_SMENR);
c4789a83 Mason Yang 2018-11-19  291  		writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
c4789a83 Mason Yang 2018-11-19  292  		ret = wait_msg_xfer_end(rpc);
c4789a83 Mason Yang 2018-11-19  293  	}
c4789a83 Mason Yang 2018-11-19  294  out:
c4789a83 Mason Yang 2018-11-19  295  	return ret;
c4789a83 Mason Yang 2018-11-19  296  }
c4789a83 Mason Yang 2018-11-19  297  
c4789a83 Mason Yang 2018-11-19  298  static void rpc_spi_mem_set_prep_op_cfg(struct spi_device *spi,
c4789a83 Mason Yang 2018-11-19  299  					const struct spi_mem_op *op,
c4789a83 Mason Yang 2018-11-19  300  					u64 *offs, size_t *len)
c4789a83 Mason Yang 2018-11-19  301  {
c4789a83 Mason Yang 2018-11-19  302  	struct rpc_spi *rpc = spi_master_get_devdata(spi->master);
c4789a83 Mason Yang 2018-11-19  303  
c4789a83 Mason Yang 2018-11-19  304  	rpc->cmd = RPC_SMCMR_CMD(op->cmd.opcode);
c4789a83 Mason Yang 2018-11-19  305  	rpc->smenr = RPC_SMENR_CDE |
c4789a83 Mason Yang 2018-11-19  306  		     RPC_SMENR_CDB(fls(op->cmd.buswidth >> 1));
c4789a83 Mason Yang 2018-11-19  307  	rpc->totalxferlen = 1;
c4789a83 Mason Yang 2018-11-19  308  	rpc->xferlen = 0;
c4789a83 Mason Yang 2018-11-19  309  	rpc->addr = 0;
c4789a83 Mason Yang 2018-11-19  310  
c4789a83 Mason Yang 2018-11-19  311  	if (op->addr.nbytes) {
c4789a83 Mason Yang 2018-11-19  312  		rpc->smenr |= RPC_SMENR_ADB(fls(op->addr.buswidth >> 1));
c4789a83 Mason Yang 2018-11-19  313  		if (op->addr.nbytes == 4)
c4789a83 Mason Yang 2018-11-19  314  			rpc->smenr |= RPC_SMENR_ADE(0xf);
c4789a83 Mason Yang 2018-11-19  315  		else
c4789a83 Mason Yang 2018-11-19  316  			rpc->smenr |= RPC_SMENR_ADE(0x7);
c4789a83 Mason Yang 2018-11-19  317  
c4789a83 Mason Yang 2018-11-19  318  		if (!offs && !len)
c4789a83 Mason Yang 2018-11-19  319  			rpc->addr = *(u32 *)offs;
c4789a83 Mason Yang 2018-11-19  320  		else
c4789a83 Mason Yang 2018-11-19  321  			rpc->addr = op->addr.val;
c4789a83 Mason Yang 2018-11-19  322  		rpc->totalxferlen += op->addr.nbytes;
c4789a83 Mason Yang 2018-11-19  323  	}
c4789a83 Mason Yang 2018-11-19  324  
c4789a83 Mason Yang 2018-11-19  325  	if (op->dummy.nbytes) {
c4789a83 Mason Yang 2018-11-19  326  		rpc->smenr |= RPC_SMENR_DME;
c4789a83 Mason Yang 2018-11-19  327  		rpc->dummy = RPC_SMDMCR_DMCYC(op->dummy.nbytes);
c4789a83 Mason Yang 2018-11-19  328  		rpc->totalxferlen += op->dummy.nbytes;
c4789a83 Mason Yang 2018-11-19  329  	}
c4789a83 Mason Yang 2018-11-19  330  
c4789a83 Mason Yang 2018-11-19  331  	if (op->data.nbytes || (offs && len)) {
c4789a83 Mason Yang 2018-11-19  332  		rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer(op->data.nbytes)) |
c4789a83 Mason Yang 2018-11-19  333  			      RPC_SMENR_SPIDB(fls(op->data.buswidth >> 1));
c4789a83 Mason Yang 2018-11-19  334  
c4789a83 Mason Yang 2018-11-19  335  		if (op->data.dir == SPI_MEM_DATA_IN) {
c4789a83 Mason Yang 2018-11-19  336  			rpc->smcr = RPC_SMCR_SPIRE;
c4789a83 Mason Yang 2018-11-19  337  			rpc->xfer_dir = SPI_MEM_DATA_IN;
c4789a83 Mason Yang 2018-11-19  338  		} else if (op->data.dir == SPI_MEM_DATA_OUT) {
c4789a83 Mason Yang 2018-11-19  339  			rpc->smcr = RPC_SMCR_SPIWE;
c4789a83 Mason Yang 2018-11-19  340  			rpc->xfer_dir = SPI_MEM_DATA_OUT;
c4789a83 Mason Yang 2018-11-19  341  		}
c4789a83 Mason Yang 2018-11-19  342  
c4789a83 Mason Yang 2018-11-19  343  		if (offs && len) {
c4789a83 Mason Yang 2018-11-19  344  			rpc->xferlen = *(u32 *)len;
c4789a83 Mason Yang 2018-11-19  345  			rpc->totalxferlen += *(u32 *)len;
c4789a83 Mason Yang 2018-11-19  346  		} else {
c4789a83 Mason Yang 2018-11-19  347  			rpc->xferlen = op->data.nbytes;
c4789a83 Mason Yang 2018-11-19  348  			rpc->totalxferlen += op->data.nbytes;
c4789a83 Mason Yang 2018-11-19  349  		}
c4789a83 Mason Yang 2018-11-19  350  	}
c4789a83 Mason Yang 2018-11-19  351  }
c4789a83 Mason Yang 2018-11-19  352  
c4789a83 Mason Yang 2018-11-19  353  static bool rpc_spi_mem_supports_op(struct spi_mem *mem,
c4789a83 Mason Yang 2018-11-19  354  				    const struct spi_mem_op *op)
c4789a83 Mason Yang 2018-11-19  355  {
c4789a83 Mason Yang 2018-11-19  356  	if (op->data.buswidth > 4 || op->addr.buswidth > 4 ||
c4789a83 Mason Yang 2018-11-19  357  	    op->dummy.buswidth > 4 || op->cmd.buswidth > 4)
c4789a83 Mason Yang 2018-11-19  358  		return false;
c4789a83 Mason Yang 2018-11-19  359  
c4789a83 Mason Yang 2018-11-19  360  	if (op->addr.nbytes > 4)
c4789a83 Mason Yang 2018-11-19  361  		return false;
c4789a83 Mason Yang 2018-11-19  362  
c4789a83 Mason Yang 2018-11-19  363  	return true;
c4789a83 Mason Yang 2018-11-19  364  }
c4789a83 Mason Yang 2018-11-19  365  
c4789a83 Mason Yang 2018-11-19  366  static ssize_t rpc_spi_mem_dirmap_read(struct spi_mem_dirmap_desc *desc,
c4789a83 Mason Yang 2018-11-19  367  				       u64 offs, size_t len, void *buf)
c4789a83 Mason Yang 2018-11-19  368  {
c4789a83 Mason Yang 2018-11-19 @369  	struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
c4789a83 Mason Yang 2018-11-19  370  	int ret;
c4789a83 Mason Yang 2018-11-19  371  
c4789a83 Mason Yang 2018-11-19 @372  	if (WARN_ON(offs + desc->info.offset + len > U32_MAX))
c4789a83 Mason Yang 2018-11-19  373  		return -EINVAL;
c4789a83 Mason Yang 2018-11-19  374  
c4789a83 Mason Yang 2018-11-19  375  	ret = rpc_spi_set_freq(rpc, desc->mem->spi->max_speed_hz);
c4789a83 Mason Yang 2018-11-19  376  	if (ret)
c4789a83 Mason Yang 2018-11-19  377  		return ret;
c4789a83 Mason Yang 2018-11-19  378  
c4789a83 Mason Yang 2018-11-19  379  	rpc_spi_mem_set_prep_op_cfg(desc->mem->spi,
c4789a83 Mason Yang 2018-11-19  380  				    &desc->info.op_tmpl, &offs, &len);
c4789a83 Mason Yang 2018-11-19  381  
c4789a83 Mason Yang 2018-11-19  382  	writel(RPC_CMNCR_SFDE | RPC_CMNCR_MOIIO_HIZ |
c4789a83 Mason Yang 2018-11-19  383  	       RPC_CMNCR_IOFV_HIZ | RPC_CMNCR_BSZ(0), rpc->regs + RPC_CMNCR);
c4789a83 Mason Yang 2018-11-19  384  
c4789a83 Mason Yang 2018-11-19  385  	writel(RPC_DRCR_RBURST(0x1f) | RPC_DRCR_RBE, rpc->regs + RPC_DRCR);
c4789a83 Mason Yang 2018-11-19  386  	writel(rpc->cmd, rpc->regs + RPC_DRCMR);
c4789a83 Mason Yang 2018-11-19  387  	writel(RPC_DREAR_EAC, rpc->regs + RPC_DREAR);
c4789a83 Mason Yang 2018-11-19  388  	writel(0, rpc->regs + RPC_DROPR);
c4789a83 Mason Yang 2018-11-19  389  	writel(rpc->smenr, rpc->regs + RPC_DRENR);
c4789a83 Mason Yang 2018-11-19  390  	writel(rpc->dummy, rpc->regs + RPC_DRDMCR);
c4789a83 Mason Yang 2018-11-19  391  	writel(0x0, rpc->regs + RPC_DRDRENR);
c4789a83 Mason Yang 2018-11-19  392  	memcpy_fromio(buf, rpc->linear.map + desc->info.offset + offs, len);
c4789a83 Mason Yang 2018-11-19  393  
c4789a83 Mason Yang 2018-11-19  394  	return len;
c4789a83 Mason Yang 2018-11-19  395  }
c4789a83 Mason Yang 2018-11-19  396  
c4789a83 Mason Yang 2018-11-19  397  static ssize_t rpc_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
c4789a83 Mason Yang 2018-11-19  398  					u64 offs, size_t len, const void *buf)
c4789a83 Mason Yang 2018-11-19  399  {
c4789a83 Mason Yang 2018-11-19 @400  	struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
c4789a83 Mason Yang 2018-11-19  401  	int tx_offs, ret;
c4789a83 Mason Yang 2018-11-19  402  
c4789a83 Mason Yang 2018-11-19  403  	if (WARN_ON(offs + desc->info.offset + len > U32_MAX))
c4789a83 Mason Yang 2018-11-19  404  		return -EINVAL;
c4789a83 Mason Yang 2018-11-19  405  
c4789a83 Mason Yang 2018-11-19  406  	if (WARN_ON(len > RPC_WBUF_SIZE))
c4789a83 Mason Yang 2018-11-19  407  		return -EIO;
c4789a83 Mason Yang 2018-11-19  408  
c4789a83 Mason Yang 2018-11-19  409  	ret = rpc_spi_set_freq(rpc, desc->mem->spi->max_speed_hz);
c4789a83 Mason Yang 2018-11-19  410  	if (ret)
c4789a83 Mason Yang 2018-11-19  411  		return ret;
c4789a83 Mason Yang 2018-11-19  412  
c4789a83 Mason Yang 2018-11-19 @413  	rpc_spi_mem_set_prep_op_cfg(desc->mem->spi,
c4789a83 Mason Yang 2018-11-19  414  				    &desc->info.op_tmpl, &offs, &len);
c4789a83 Mason Yang 2018-11-19  415  
c4789a83 Mason Yang 2018-11-19  416  	writel(RPC_CMNCR_MD | RPC_CMNCR_SFDE | RPC_CMNCR_MOIIO_HIZ |
c4789a83 Mason Yang 2018-11-19  417  	       RPC_CMNCR_IOFV_HIZ | RPC_CMNCR_BSZ(0), rpc->regs + RPC_CMNCR);
c4789a83 Mason Yang 2018-11-19  418  	writel(0x0, rpc->regs + RPC_SMDRENR);
c4789a83 Mason Yang 2018-11-19  419  
c4789a83 Mason Yang 2018-11-19  420  	writel(RPC_PHYCNT_CAL | 0x260 | RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF,
c4789a83 Mason Yang 2018-11-19  421  	       rpc->regs + RPC_PHYCNT);
c4789a83 Mason Yang 2018-11-19  422  
c4789a83 Mason Yang 2018-11-19  423  	for (tx_offs = 0; tx_offs < RPC_WBUF_SIZE; tx_offs += 4)
c4789a83 Mason Yang 2018-11-19  424  		writel(*(u32 *)(buf + tx_offs), rpc->regs + RPC_WBUF + tx_offs);
c4789a83 Mason Yang 2018-11-19  425  
c4789a83 Mason Yang 2018-11-19  426  	writel(rpc->cmd, rpc->regs + RPC_SMCMR);
c4789a83 Mason Yang 2018-11-19  427  	writel(offs, rpc->regs + RPC_SMADR);
c4789a83 Mason Yang 2018-11-19  428  	writel(rpc->smenr, rpc->regs + RPC_SMENR);
c4789a83 Mason Yang 2018-11-19  429  	writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
c4789a83 Mason Yang 2018-11-19  430  	ret = wait_msg_xfer_end(rpc);
c4789a83 Mason Yang 2018-11-19  431  	if (ret)
c4789a83 Mason Yang 2018-11-19  432  		goto out;
c4789a83 Mason Yang 2018-11-19  433  
c4789a83 Mason Yang 2018-11-19  434  	writel(RPC_DRCR_RCF, rpc->regs + RPC_DRCR);
c4789a83 Mason Yang 2018-11-19  435  	writel(RPC_PHYCNT_CAL | RPC_PHYCNT_STRTIM(0) | 0x260,
c4789a83 Mason Yang 2018-11-19  436  	       rpc->regs + RPC_PHYCNT);
c4789a83 Mason Yang 2018-11-19  437  
c4789a83 Mason Yang 2018-11-19  438  	return len;
c4789a83 Mason Yang 2018-11-19  439  out:
c4789a83 Mason Yang 2018-11-19  440  	return ret;
c4789a83 Mason Yang 2018-11-19  441  }
c4789a83 Mason Yang 2018-11-19  442  
c4789a83 Mason Yang 2018-11-19  443  static int rpc_spi_mem_dirmap_create(struct spi_mem_dirmap_desc *desc)
c4789a83 Mason Yang 2018-11-19  444  {
c4789a83 Mason Yang 2018-11-19  445  	struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
c4789a83 Mason Yang 2018-11-19  446  
c4789a83 Mason Yang 2018-11-19  447  	if (desc->info.offset + desc->info.length > U32_MAX)
c4789a83 Mason Yang 2018-11-19  448  		return -ENOTSUPP;
c4789a83 Mason Yang 2018-11-19  449  
c4789a83 Mason Yang 2018-11-19  450  	if (!rpc_spi_mem_supports_op(desc->mem, &desc->info.op_tmpl))
c4789a83 Mason Yang 2018-11-19  451  		return -ENOTSUPP;
c4789a83 Mason Yang 2018-11-19  452  
c4789a83 Mason Yang 2018-11-19  453  	if (!rpc->linear.map &&
c4789a83 Mason Yang 2018-11-19  454  	    desc->info.op_tmpl.data.dir == SPI_MEM_DATA_IN)
c4789a83 Mason Yang 2018-11-19  455  		return -ENOTSUPP;
c4789a83 Mason Yang 2018-11-19  456  
c4789a83 Mason Yang 2018-11-19  457  	return 0;
c4789a83 Mason Yang 2018-11-19  458  }
c4789a83 Mason Yang 2018-11-19  459  
c4789a83 Mason Yang 2018-11-19  460  static int rpc_spi_mem_exec_op(struct spi_mem *mem,
c4789a83 Mason Yang 2018-11-19  461  			       const struct spi_mem_op *op)
c4789a83 Mason Yang 2018-11-19  462  {
c4789a83 Mason Yang 2018-11-19  463  	struct rpc_spi *rpc = spi_master_get_devdata(mem->spi->master);
c4789a83 Mason Yang 2018-11-19  464  	int ret;
c4789a83 Mason Yang 2018-11-19  465  
c4789a83 Mason Yang 2018-11-19  466  	ret = rpc_spi_set_freq(rpc, mem->spi->max_speed_hz);
c4789a83 Mason Yang 2018-11-19  467  	if (ret)
c4789a83 Mason Yang 2018-11-19  468  		return ret;
c4789a83 Mason Yang 2018-11-19  469  
c4789a83 Mason Yang 2018-11-19  470  	rpc_spi_mem_set_prep_op_cfg(mem->spi, op, NULL, NULL);
c4789a83 Mason Yang 2018-11-19  471  
c4789a83 Mason Yang 2018-11-19  472  	ret = rpc_spi_io_xfer(rpc,
c4789a83 Mason Yang 2018-11-19  473  			      op->data.dir == SPI_MEM_DATA_OUT ?
c4789a83 Mason Yang 2018-11-19  474  			      op->data.buf.out : NULL,
c4789a83 Mason Yang 2018-11-19  475  			      op->data.dir == SPI_MEM_DATA_IN ?
c4789a83 Mason Yang 2018-11-19  476  			      op->data.buf.in : NULL);
c4789a83 Mason Yang 2018-11-19  477  
c4789a83 Mason Yang 2018-11-19  478  	return ret;
c4789a83 Mason Yang 2018-11-19  479  }
c4789a83 Mason Yang 2018-11-19  480  
c4789a83 Mason Yang 2018-11-19  481  static const struct spi_controller_mem_ops rpc_spi_mem_ops = {
c4789a83 Mason Yang 2018-11-19  482  	.supports_op = rpc_spi_mem_supports_op,
c4789a83 Mason Yang 2018-11-19  483  	.exec_op = rpc_spi_mem_exec_op,
c4789a83 Mason Yang 2018-11-19 @484  	.dirmap_create = rpc_spi_mem_dirmap_create,
c4789a83 Mason Yang 2018-11-19  485  	.dirmap_read = rpc_spi_mem_dirmap_read,
c4789a83 Mason Yang 2018-11-19  486  	.dirmap_write = rpc_spi_mem_dirmap_write,
c4789a83 Mason Yang 2018-11-19  487  };
c4789a83 Mason Yang 2018-11-19  488  

:::::: The code at line 369 was first introduced by commit
:::::: c4789a83a8be18f144419fba933a3f5ca1b78837 spi: Add Renesas R-Car RPC SPI controller driver

:::::: TO: Mason Yang <masonccyang@mxic.com.tw>
:::::: CC: 0day robot <lkp@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 66636 bytes --]