Hi Mark,
This Renesas R-Car Gen3 RPC SPI driver is based on Boris's new
spi-mem direct mapping read/write mode [1][2] and v2 patch is from
Marek and Geert's comments including RPC clock control,
run time PM, RPC module software reset, regmap and so on.
thanks for your review.
best regards,
Mason
[1] https://patchwork.kernel.org/patch/10670753
[2] https://patchwork.kernel.org/patch/10670747
Mason Yang (2):
spi: Add Renesas R-Car Gen3 RPC SPI controller driver
dt-binding: spi: Document Renesas R-Car Gen3 RPC controller bindings
.../devicetree/bindings/spi/spi-renesas-rpc.txt | 35 +
drivers/spi/Kconfig | 6 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-renesas-rpc.c | 808 +++++++++++++++++++++
4 files changed, 850 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
create mode 100644 drivers/spi/spi-renesas-rpc.c
--
1.9.1
Document the bindings used by the Renesas R-Car Gen3 RPC SPI controller.
Signed-off-by: Mason Yang <[email protected]>
---
.../devicetree/bindings/spi/spi-renesas-rpc.txt | 35 ++++++++++++++++++++++
1 file changed, 35 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..c6c6d9c
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
@@ -0,0 +1,35 @@
+Renesas R-Car Gen3 RPC controller Device Tree Bindings
+------------------------------------------------------
+
+Required properties:
+- compatible: should be "renesas,r8a77995-rpc"
+- #address-cells: should be 1
+- #size-cells: should be 0
+- reg: should contain 2 entries, one for the registers and one for the direct
+ mapping area
+- reg-names: should contain "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 module's clock
+
+Example:
+
+ rpc: spi@ee200000 {
+ compatible = "renesas,r8a77995-rpc";
+ reg = <0 0xee200000 0 0x8100>, <0 0x08000000 0 0x4000000>;
+ reg-names = "rpc_regs", "dirmap";
+ clocks = <&cpg CPG_MOD 917>;
+ power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;
+ resets = <&cpg 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
Add a driver for Renesas R-Car Gen3 RPC SPI controller.
Signed-off-by: Mason Yang <[email protected]>
---
drivers/spi/Kconfig | 6 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-renesas-rpc.c | 808 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 815 insertions(+)
create mode 100644 drivers/spi/spi-renesas-rpc.c
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 7d3a5c9..8f826fe 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -528,6 +528,12 @@ config SPI_RSPI
help
SPI driver for Renesas RSPI and QSPI blocks.
+config SPI_RENESAS_RPC
+ tristate "Renesas R-Car Gen3 RPC SPI controller"
+ depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
+ help
+ SPI driver for Renesas R-Car Gen3 RPC.
+
config SPI_QCOM_QSPI
tristate "QTI QSPI controller"
depends on ARCH_QCOM
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 3575205..5d5c523 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -81,6 +81,7 @@ obj-$(CONFIG_SPI_QUP) += spi-qup.o
obj-$(CONFIG_SPI_ROCKCHIP) += spi-rockchip.o
obj-$(CONFIG_SPI_RB4XX) += spi-rb4xx.o
obj-$(CONFIG_SPI_RSPI) += spi-rspi.o
+obj-$(CONFIG_SPI_RENESAS_RPC) += spi-renesas-rpc.o
obj-$(CONFIG_SPI_S3C24XX) += spi-s3c24xx-hw.o
spi-s3c24xx-hw-y := spi-s3c24xx.o
spi-s3c24xx-hw-$(CONFIG_SPI_S3C24XX_FIQ) += spi-s3c24xx-fiq.o
diff --git a/drivers/spi/spi-renesas-rpc.c b/drivers/spi/spi-renesas-rpc.c
new file mode 100644
index 0000000..ac9094e
--- /dev/null
+++ b/drivers/spi/spi-renesas-rpc.c
@@ -0,0 +1,808 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (C) 2018 ~ 2019 Renesas Solutions Corp.
+// Copyright (C) 2018 Macronix International Co., Ltd.
+//
+// R-Car Gen3 RPC SPI/QSPI/Octa driver
+//
+// Authors:
+// Mason Yang <[email protected]>
+//
+
+#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/regmap.h>
+#include <linux/reset.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 */
+
+#define LOOP_TIMEOUT 1024
+
+struct rpc_spi {
+ struct clk *clk_rpc;
+ void __iomem *base;
+ struct {
+ void __iomem *map;
+ dma_addr_t dma;
+ size_t size;
+ } linear;
+ struct regmap *regmap;
+ u32 cur_speed_hz;
+ u32 cmd;
+ u32 addr;
+ u32 dummy;
+ u32 smcr;
+ u32 smenr;
+ u32 xferlen;
+ u32 totalxferlen;
+ enum spi_mem_data_dir xfer_dir;
+#ifdef CONFIG_RESET_CONTROLLER
+ struct reset_control *rstc;
+#endif
+};
+
+static int rpc_spi_set_freq(struct rpc_spi *rpc, unsigned long freq)
+{
+ int ret;
+
+ if (rpc->cur_speed_hz == freq)
+ return 0;
+
+ ret = clk_set_rate(rpc->clk_rpc, freq);
+ if (ret)
+ return ret;
+
+ rpc->cur_speed_hz = freq;
+ return ret;
+}
+
+static void rpc_spi_hw_init(struct rpc_spi *rpc)
+{
+ /*
+ * NOTE: The 0x260 are undocumented bits, but they must be set.
+ * RPC_PHYCNT_STRTIM is strobe timing adjustment bit,
+ * 0x0 : the delay is biggest,
+ * 0x1 : the delay is 2nd biggest,
+ * 0x3 or 0x6 is a recommended value.
+ */
+ regmap_write(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_CAL |
+ RPC_PHYCNT_STRTIM(0x6) | 0x260);
+
+ /*
+ * NOTE: The 0x31511144 and 0x431 are undocumented bits,
+ * but they must be set for RPC_PHYOFFSET1 & RPC_PHYOFFSET2.
+ */
+ regmap_write(rpc->regmap, RPC_PHYOFFSET1, 0x31511144);
+ regmap_write(rpc->regmap, RPC_PHYOFFSET2, 0x431);
+
+ regmap_write(rpc->regmap, RPC_SSLDR, RPC_SSLDR_SPNDL(7) |
+ RPC_SSLDR_SLNDL(7) | RPC_SSLDR_SCKDL(7));
+}
+
+#ifdef CONFIG_RESET_CONTROLLER
+static int rpc_spi_do_reset(struct rpc_spi *rpc)
+{
+ int i, ret;
+
+ ret = reset_control_reset(rpc->rstc);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < LOOP_TIMEOUT; i++) {
+ ret = reset_control_status(rpc->rstc);
+ if (ret == 0)
+ return 0;
+ usleep_range(0, 1);
+ }
+
+ return -ETIMEDOUT;
+}
+#else
+static int rpc_spi_do_reset(struct rpc_spi *rpc)
+{
+ return -ETIMEDOUT;
+}
+#endif
+
+static int wait_msg_xfer_end(struct rpc_spi *rpc)
+{
+ u32 sts;
+
+ return regmap_read_poll_timeout(rpc->regmap, RPC_CMNSR, sts,
+ sts & RPC_CMNSR_TEND, 0, USEC_PER_SEC);
+}
+
+static u8 rpc_bits_xfer(u32 nbytes)
+{
+ if (nbytes > 4)
+ nbytes = 4;
+
+ return GENMASK(3, 4 - nbytes);
+}
+
+static int rpc_spi_io_xfer(struct rpc_spi *rpc,
+ const void *tx_buf, void *rx_buf)
+{
+ u32 smenr, smcr, data, pos = 0;
+ int ret = 0;
+
+ regmap_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD | RPC_CMNCR_SFDE |
+ RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
+ RPC_CMNCR_BSZ(0));
+ regmap_write(rpc->regmap, RPC_SMDRENR, 0x0);
+ regmap_write(rpc->regmap, RPC_SMCMR, rpc->cmd);
+ regmap_write(rpc->regmap, RPC_SMDMCR, rpc->dummy);
+ regmap_write(rpc->regmap, RPC_SMADR, rpc->addr);
+
+ if (tx_buf) {
+ smenr = rpc->smenr;
+
+ while (pos < rpc->xferlen) {
+ u32 nbytes = rpc->xferlen - pos;
+
+ regmap_write(rpc->regmap, RPC_SMWDR0,
+ *(u32 *)(tx_buf + pos));
+
+ if (nbytes > 4) {
+ nbytes = 4;
+ smcr = rpc->smcr |
+ RPC_SMCR_SPIE | RPC_SMCR_SSLKP;
+ } else {
+ smcr = rpc->smcr | RPC_SMCR_SPIE;
+ }
+
+ regmap_write(rpc->regmap, RPC_SMENR, smenr);
+ regmap_write(rpc->regmap, RPC_SMCR, smcr);
+ ret = wait_msg_xfer_end(rpc);
+ if (ret)
+ goto out;
+
+ pos += nbytes;
+ smenr = rpc->smenr & ~RPC_SMENR_CDE &
+ ~RPC_SMENR_ADE(0xf);
+ }
+ } else if (rx_buf) {
+ while (pos < rpc->xferlen) {
+ u32 nbytes = rpc->xferlen - pos;
+
+ if (nbytes > 4)
+ nbytes = 4;
+
+ regmap_write(rpc->regmap, RPC_SMENR, rpc->smenr);
+ regmap_write(rpc->regmap, RPC_SMCR,
+ rpc->smcr | RPC_SMCR_SPIE);
+ ret = wait_msg_xfer_end(rpc);
+ if (ret)
+ goto out;
+
+ regmap_read(rpc->regmap, RPC_SMRDR0, &data);
+ memcpy_fromio(rx_buf + pos, (void *)&data, nbytes);
+ pos += nbytes;
+ regmap_write(rpc->regmap, RPC_SMCMR, rpc->cmd);
+ regmap_write(rpc->regmap, RPC_SMDMCR, rpc->dummy);
+ regmap_write(rpc->regmap, RPC_SMADR, rpc->addr + pos);
+ }
+ } else {
+ regmap_write(rpc->regmap, RPC_SMENR, rpc->smenr);
+ regmap_write(rpc->regmap, RPC_SMCR, rpc->smcr | RPC_SMCR_SPIE);
+ ret = wait_msg_xfer_end(rpc);
+ if (ret)
+ goto out;
+ }
+
+ return ret;
+out:
+ return rpc_spi_do_reset(rpc);
+}
+
+static void rpc_spi_mem_set_prep_op_cfg(struct spi_device *spi,
+ const struct spi_mem_op *op,
+ u64 *offs, size_t *len)
+{
+ struct rpc_spi *rpc = spi_master_get_devdata(spi->master);
+
+ rpc->cmd = RPC_SMCMR_CMD(op->cmd.opcode);
+ rpc->smenr = RPC_SMENR_CDE |
+ RPC_SMENR_CDB(fls(op->cmd.buswidth >> 1));
+ rpc->totalxferlen = 1;
+ rpc->xfer_dir = SPI_MEM_NO_DATA;
+ rpc->xferlen = 0;
+ rpc->addr = 0;
+
+ if (op->addr.nbytes) {
+ rpc->smenr |= RPC_SMENR_ADB(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)) {
+ if (op->data.dir == SPI_MEM_DATA_IN) {
+ rpc->smcr = RPC_SMCR_SPIRE;
+ rpc->xfer_dir = SPI_MEM_DATA_IN;
+ } else if (op->data.dir == SPI_MEM_DATA_OUT) {
+ rpc->smcr = RPC_SMCR_SPIWE;
+ rpc->xfer_dir = SPI_MEM_DATA_OUT;
+ }
+
+ if (offs && len) {
+ rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer
+ (*(u32 *)len)) | RPC_SMENR_SPIDB
+ (fls(op->data.buswidth >> 1));
+
+ rpc->xferlen = *(u32 *)len;
+ rpc->totalxferlen += *(u32 *)len;
+ } else {
+ rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer
+ (op->data.nbytes)) | RPC_SMENR_SPIDB
+ (fls(op->data.buswidth >> 1));
+
+ 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);
+
+ regmap_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_SFDE |
+ RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
+ RPC_CMNCR_BSZ(0));
+ regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RBURST(0x1f) |
+ RPC_DRCR_RBE);
+ regmap_write(rpc->regmap, RPC_DRCMR, rpc->cmd);
+ regmap_write(rpc->regmap, RPC_DREAR, RPC_DREAR_EAC);
+ regmap_write(rpc->regmap, RPC_DROPR, 0);
+ regmap_write(rpc->regmap, RPC_DRENR, rpc->smenr);
+ regmap_write(rpc->regmap, RPC_DRDMCR, rpc->dummy);
+ regmap_write(rpc->regmap, RPC_DRDRENR, 0);
+
+ memcpy_fromio(buf, rpc->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 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);
+
+ regmap_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD | RPC_CMNCR_SFDE |
+ RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
+ RPC_CMNCR_BSZ(0));
+ regmap_write(rpc->regmap, RPC_SMDRENR, 0);
+ regmap_write(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_CAL | 0x260 |
+ RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF);
+
+ memcpy_toio(rpc->base + RPC_WBUF, buf, len);
+
+ regmap_write(rpc->regmap, RPC_SMCMR, rpc->cmd);
+ regmap_write(rpc->regmap, RPC_SMADR, offs);
+ regmap_write(rpc->regmap, RPC_SMENR, rpc->smenr);
+ regmap_write(rpc->regmap, RPC_SMCR, rpc->smcr | RPC_SMCR_SPIE);
+ ret = wait_msg_xfer_end(rpc);
+ if (ret)
+ goto out;
+
+ regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RCF);
+ regmap_write(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_CAL |
+ RPC_PHYCNT_STRTIM(6) | 0x260);
+
+ return len;
+out:
+ return rpc_spi_do_reset(rpc);
+}
+
+static int rpc_spi_mem_dirmap_create(struct spi_mem_dirmap_desc *desc)
+{
+ struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
+
+ if (desc->info.offset + desc->info.length > U32_MAX)
+ return -ENOTSUPP;
+
+ if (!rpc_spi_mem_supports_op(desc->mem, &desc->info.op_tmpl))
+ return -ENOTSUPP;
+
+ if (!rpc->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;
+ rpc->xfer_dir = SPI_MEM_NO_DATA;
+
+ list_for_each_entry(t, &msg->transfers, transfer_list) {
+ if (t->tx_buf) {
+ xfer[xferpos].tx_buf = t->tx_buf;
+ xfer[xferpos].tx_nbits = t->tx_nbits;
+ }
+
+ if (t->rx_buf) {
+ xfer[xferpos].rx_buf = t->rx_buf;
+ xfer[xferpos].rx_nbits = t->rx_nbits;
+ }
+
+ if (t->len) {
+ xfer[xferpos++].len = t->len;
+ rpc->totalxferlen += t->len;
+ }
+
+ if (list_is_last(&t->transfer_list, &msg->transfers)) {
+ if (xferpos > 1 && t->rx_buf) {
+ rpc->xfer_dir = SPI_MEM_DATA_IN;
+ rpc->smcr = RPC_SMCR_SPIRE;
+ } else if (xferpos > 1 && t->tx_buf) {
+ rpc->xfer_dir = SPI_MEM_DATA_OUT;
+ rpc->smcr = RPC_SMCR_SPIWE;
+ }
+ }
+ }
+
+ xfercnt = xferpos;
+ rpc->xferlen = xfer[--xferpos].len;
+ rpc->cmd = RPC_SMCMR_CMD(((u8 *)xfer[0].tx_buf)[0]);
+ rpc->smenr = RPC_SMENR_CDE | RPC_SMENR_CDB(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));
+ } 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));
+ }
+ 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));
+ } 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));
+ }
+ break;
+
+ case 4:
+ if (xfer[2].len && xfer[2].tx_buf) {
+ rpc->smenr |= RPC_SMENR_DME;
+ rpc->dummy = RPC_SMDMCR_DMCYC(xfer[2].len);
+ }
+
+ 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));
+ }
+ 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))
+ continue;
+ ret = rpc_spi_xfer_message(rpc, t);
+ if (ret)
+ goto out;
+ }
+
+ msg->status = 0;
+ msg->actual_length = rpc->totalxferlen;
+out:
+ spi_finalize_current_message(master);
+ return 0;
+}
+
+static const struct regmap_range rpc_spi_volatile_ranges[] = {
+ regmap_reg_range(RPC_SMRDR0, RPC_SMRDR0),
+ regmap_reg_range(RPC_SMWDR0, RPC_SMWDR0),
+ regmap_reg_range(RPC_CMNSR, RPC_CMNSR),
+};
+
+static const struct regmap_access_table rpc_spi_volatile_table = {
+ .yes_ranges = rpc_spi_volatile_ranges,
+ .n_yes_ranges = ARRAY_SIZE(rpc_spi_volatile_ranges),
+};
+
+static const struct regmap_config rpc_spi_regmap_config = {
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = 4,
+ .fast_io = true,
+ .max_register = RPC_WBUF + RPC_WBUF_SIZE,
+ .volatile_table = &rpc_spi_volatile_table,
+};
+
+static int rpc_spi_probe(struct platform_device *pdev)
+{
+ struct spi_master *master;
+ struct resource *res;
+ struct rpc_spi *rpc;
+ const struct regmap_config *regmap_config;
+ 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->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(rpc->base))
+ return PTR_ERR(rpc->base);
+
+ regmap_config = &rpc_spi_regmap_config;
+ rpc->regmap = devm_regmap_init_mmio(&pdev->dev, rpc->base,
+ regmap_config);
+ if (IS_ERR(rpc->regmap)) {
+ dev_err(&pdev->dev, "failed to init regmap %ld for rpc-spi\n",
+ PTR_ERR(rpc->regmap));
+ return PTR_ERR(rpc->regmap);
+ }
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dirmap");
+ rpc->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;
+ }
+
+ rpc->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+ if (IS_ERR(rpc->rstc))
+ return PTR_ERR(rpc->rstc);
+
+ pm_runtime_enable(&pdev->dev);
+ master->auto_runtime_pm = true;
+
+ master->num_chipselect = 1;
+ master->mem_ops = &rpc_spi_mem_ops;
+ master->transfer_one_message = rpc_spi_transfer_one_message;
+
+ master->bits_per_word_mask = SPI_BPW_MASK(8);
+ master->mode_bits = SPI_CPOL | SPI_CPHA |
+ 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,r8a77995-rpc", },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rpc_spi_of_ids);
+
+#ifdef CONFIG_PM_SLEEP
+static int rpc_spi_suspend(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct spi_master *master = platform_get_drvdata(pdev);
+
+ return spi_master_suspend(master);
+}
+
+static int rpc_spi_resume(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct spi_master *master = platform_get_drvdata(pdev);
+
+ return spi_master_resume(master);
+}
+
+static SIMPLE_DEV_PM_OPS(rpc_spi_pm_ops, rpc_spi_suspend, rpc_spi_resume);
+#define DEV_PM_OPS (&rpc_spi_pm_ops)
+#else
+#define DEV_PM_OPS NULL
+#endif
+
+static struct platform_driver rpc_spi_driver = {
+ .probe = rpc_spi_probe,
+ .remove = rpc_spi_remove,
+ .driver = {
+ .name = "rpc-spi",
+ .of_match_table = rpc_spi_of_ids,
+ .pm = DEV_PM_OPS,
+ },
+};
+module_platform_driver(rpc_spi_driver);
+
+MODULE_AUTHOR("Mason Yang <[email protected]>");
+MODULE_DESCRIPTION("Renesas R-Car Gen3 RPC SPI controller driver");
+MODULE_LICENSE("GPL v2");
--
1.9.1
On 12/03/2018 10:18 AM, Mason Yang wrote:
> Document the bindings used by the Renesas R-Car Gen3 RPC SPI controller.
RPC is SPI and HF controller, it is not a pure SPI controller.
How does this deal with the HF part ? Keep in mind the bindings are ABI
and it will be difficult to redo them later.
> Signed-off-by: Mason Yang <[email protected]>
> ---
> .../devicetree/bindings/spi/spi-renesas-rpc.txt | 35 ++++++++++++++++++++++
> 1 file changed, 35 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..c6c6d9c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> @@ -0,0 +1,35 @@
> +Renesas R-Car Gen3 RPC controller Device Tree Bindings
> +------------------------------------------------------
> +
> +Required properties:
> +- compatible: should be "renesas,r8a77995-rpc"
> +- #address-cells: should be 1
> +- #size-cells: should be 0
> +- reg: should contain 2 entries, one for the registers and one for the direct
> + mapping area
> +- reg-names: should contain "rpc_regs" and "dirmap"
> +- interrupts: interrupt line connected to the RPC SPI controller
Not a SPI controller.
> +- clock-names: should contain "clk_rpc"
> +- clocks: should contain 1 entries for the module's clock
> +
> +Example:
> +
> + rpc: spi@ee200000 {
> + compatible = "renesas,r8a77995-rpc";
> + reg = <0 0xee200000 0 0x8100>, <0 0x08000000 0 0x4000000>;
> + reg-names = "rpc_regs", "dirmap";
> + clocks = <&cpg CPG_MOD 917>;
> + power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;
> + resets = <&cpg 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
On 12/03/2018 10:18 AM, Mason Yang wrote:
> Add a driver for Renesas R-Car Gen3 RPC SPI controller.
>
> Signed-off-by: Mason Yang <[email protected]>
What changed in this V2 ?
[...]
> +struct rpc_spi {
> + struct clk *clk_rpc;
> + void __iomem *base;
> + struct {
> + void __iomem *map;
> + dma_addr_t dma;
> + size_t size;
> + } linear;
This is still here, see my review comments on the previous version.
> + struct regmap *regmap;
> + u32 cur_speed_hz;
> + u32 cmd;
> + u32 addr;
> + u32 dummy;
> + u32 smcr;
> + u32 smenr;
> + u32 xferlen;
> + u32 totalxferlen;
> + enum spi_mem_data_dir xfer_dir;
> +#ifdef CONFIG_RESET_CONTROLLER
> + struct reset_control *rstc;
> +#endif
> +};
> +
> +static int rpc_spi_set_freq(struct rpc_spi *rpc, unsigned long freq)
> +{
> + int ret;
> +
> + if (rpc->cur_speed_hz == freq)
> + return 0;
> +
> + ret = clk_set_rate(rpc->clk_rpc, freq);
> + if (ret)
> + return ret;
> +
> + rpc->cur_speed_hz = freq;
> + return ret;
> +}
> +
> +static void rpc_spi_hw_init(struct rpc_spi *rpc)
> +{
> + /*
> + * NOTE: The 0x260 are undocumented bits, but they must be set.
> + * RPC_PHYCNT_STRTIM is strobe timing adjustment bit,
> + * 0x0 : the delay is biggest,
> + * 0x1 : the delay is 2nd biggest,
> + * 0x3 or 0x6 is a recommended value.
> + */
Doesn't this vary by SoC ? I think H3 ES1.0 had different value here,
but I might be wrong.
> + regmap_write(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_CAL |
> + RPC_PHYCNT_STRTIM(0x6) | 0x260);
> +
> + /*
> + * NOTE: The 0x31511144 and 0x431 are undocumented bits,
> + * but they must be set for RPC_PHYOFFSET1 & RPC_PHYOFFSET2.
> + */
> + regmap_write(rpc->regmap, RPC_PHYOFFSET1, 0x31511144);
> + regmap_write(rpc->regmap, RPC_PHYOFFSET2, 0x431);
> +
> + regmap_write(rpc->regmap, RPC_SSLDR, RPC_SSLDR_SPNDL(7) |
> + RPC_SSLDR_SLNDL(7) | RPC_SSLDR_SCKDL(7));
> +}
> +
> +#ifdef CONFIG_RESET_CONTROLLER
Just make the driver depend on reset controller.
> +static int rpc_spi_do_reset(struct rpc_spi *rpc)
> +{
> + int i, ret;
> +
> + ret = reset_control_reset(rpc->rstc);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < LOOP_TIMEOUT; i++) {
> + ret = reset_control_status(rpc->rstc);
> + if (ret == 0)
> + return 0;
> + usleep_range(0, 1);
> + }
> +
> + return -ETIMEDOUT;
> +}
> +#else
> +static int rpc_spi_do_reset(struct rpc_spi *rpc)
> +{
> + return -ETIMEDOUT;
> +}
> +#endif
> +
> +static int wait_msg_xfer_end(struct rpc_spi *rpc)
> +{
> + u32 sts;
> +
> + return regmap_read_poll_timeout(rpc->regmap, RPC_CMNSR, sts,
> + sts & RPC_CMNSR_TEND, 0, USEC_PER_SEC);
> +}
> +
> +static u8 rpc_bits_xfer(u32 nbytes)
> +{
> + if (nbytes > 4)
> + nbytes = 4;
Use clamp() ?
> +
> + return GENMASK(3, 4 - nbytes);
Nice ;-)
> +}
> +
> +static int rpc_spi_io_xfer(struct rpc_spi *rpc,
> + const void *tx_buf, void *rx_buf)
> +{
> + u32 smenr, smcr, data, pos = 0;
> + int ret = 0;
> +
> + regmap_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD | RPC_CMNCR_SFDE |
> + RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
> + RPC_CMNCR_BSZ(0));
> + regmap_write(rpc->regmap, RPC_SMDRENR, 0x0);
> + regmap_write(rpc->regmap, RPC_SMCMR, rpc->cmd);
> + regmap_write(rpc->regmap, RPC_SMDMCR, rpc->dummy);
> + regmap_write(rpc->regmap, RPC_SMADR, rpc->addr);
> +
> + if (tx_buf) {
> + smenr = rpc->smenr;
> +
> + while (pos < rpc->xferlen) {
> + u32 nbytes = rpc->xferlen - pos;
> +
> + regmap_write(rpc->regmap, RPC_SMWDR0,
> + *(u32 *)(tx_buf + pos));
*(u32 *) cast is probably not needed , fix casts globally.
> + if (nbytes > 4) {
> + nbytes = 4;
> + smcr = rpc->smcr |
> + RPC_SMCR_SPIE | RPC_SMCR_SSLKP;
> + } else {
> + smcr = rpc->smcr | RPC_SMCR_SPIE;
> + }
> +
> + regmap_write(rpc->regmap, RPC_SMENR, smenr);
> + regmap_write(rpc->regmap, RPC_SMCR, smcr);
> + ret = wait_msg_xfer_end(rpc);
> + if (ret)
> + goto out;
> +
> + pos += nbytes;
> + smenr = rpc->smenr & ~RPC_SMENR_CDE &
> + ~RPC_SMENR_ADE(0xf);
> + }
> + } else if (rx_buf) {
> + while (pos < rpc->xferlen) {
> + u32 nbytes = rpc->xferlen - pos;
> +
> + if (nbytes > 4)
> + nbytes = 4;
> +
> + regmap_write(rpc->regmap, RPC_SMENR, rpc->smenr);
> + regmap_write(rpc->regmap, RPC_SMCR,
> + rpc->smcr | RPC_SMCR_SPIE);
> + ret = wait_msg_xfer_end(rpc);
> + if (ret)
> + goto out;
> +
> + regmap_read(rpc->regmap, RPC_SMRDR0, &data);
> + memcpy_fromio(rx_buf + pos, (void *)&data, nbytes);
> + pos += nbytes;
> + regmap_write(rpc->regmap, RPC_SMCMR, rpc->cmd);
> + regmap_write(rpc->regmap, RPC_SMDMCR, rpc->dummy);
> + regmap_write(rpc->regmap, RPC_SMADR, rpc->addr + pos);
> + }
> + } else {
> + regmap_write(rpc->regmap, RPC_SMENR, rpc->smenr);
> + regmap_write(rpc->regmap, RPC_SMCR, rpc->smcr | RPC_SMCR_SPIE);
> + ret = wait_msg_xfer_end(rpc);
> + if (ret)
> + goto out;
> + }
> +
> + return ret;
> +out:
> + return rpc_spi_do_reset(rpc);
> +}
> +
> +static void rpc_spi_mem_set_prep_op_cfg(struct spi_device *spi,
> + const struct spi_mem_op *op,
> + u64 *offs, size_t *len)
> +{
> + struct rpc_spi *rpc = spi_master_get_devdata(spi->master);
> +
> + rpc->cmd = RPC_SMCMR_CMD(op->cmd.opcode);
> + rpc->smenr = RPC_SMENR_CDE |
> + RPC_SMENR_CDB(fls(op->cmd.buswidth >> 1));
> + rpc->totalxferlen = 1;
> + rpc->xfer_dir = SPI_MEM_NO_DATA;
> + rpc->xferlen = 0;
> + rpc->addr = 0;
> +
> + if (op->addr.nbytes) {
> + rpc->smenr |= RPC_SMENR_ADB(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)) {
> + if (op->data.dir == SPI_MEM_DATA_IN) {
> + rpc->smcr = RPC_SMCR_SPIRE;
> + rpc->xfer_dir = SPI_MEM_DATA_IN;
> + } else if (op->data.dir == SPI_MEM_DATA_OUT) {
> + rpc->smcr = RPC_SMCR_SPIWE;
> + rpc->xfer_dir = SPI_MEM_DATA_OUT;
> + }
> +
> + if (offs && len) {
> + rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer
> + (*(u32 *)len)) | RPC_SMENR_SPIDB
> + (fls(op->data.buswidth >> 1));
Fix the *(u32 *)
> + rpc->xferlen = *(u32 *)len;
> + rpc->totalxferlen += *(u32 *)len;
> + } else {
> + rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer
> + (op->data.nbytes)) | RPC_SMENR_SPIDB
> + (fls(op->data.buswidth >> 1));
Drop parenthesis around fls()
> +
> + 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)
Merge this into previous conditional statement.
> + 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);
> +
> + regmap_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_SFDE |
> + RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
> + RPC_CMNCR_BSZ(0));
> + regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RBURST(0x1f) |
> + RPC_DRCR_RBE);
> + regmap_write(rpc->regmap, RPC_DRCMR, rpc->cmd);
> + regmap_write(rpc->regmap, RPC_DREAR, RPC_DREAR_EAC);
> + regmap_write(rpc->regmap, RPC_DROPR, 0);
> + regmap_write(rpc->regmap, RPC_DRENR, rpc->smenr);
> + regmap_write(rpc->regmap, RPC_DRDMCR, rpc->dummy);
> + regmap_write(rpc->regmap, RPC_DRDRENR, 0);
> +
> + memcpy_fromio(buf, rpc->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 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);
> +
> + regmap_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD | RPC_CMNCR_SFDE |
> + RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
> + RPC_CMNCR_BSZ(0));
> + regmap_write(rpc->regmap, RPC_SMDRENR, 0);
> + regmap_write(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_CAL | 0x260 |
> + RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF);
> +
> + memcpy_toio(rpc->base + RPC_WBUF, buf, len);
> +
> + regmap_write(rpc->regmap, RPC_SMCMR, rpc->cmd);
> + regmap_write(rpc->regmap, RPC_SMADR, offs);
> + regmap_write(rpc->regmap, RPC_SMENR, rpc->smenr);
> + regmap_write(rpc->regmap, RPC_SMCR, rpc->smcr | RPC_SMCR_SPIE);
> + ret = wait_msg_xfer_end(rpc);
> + if (ret)
> + goto out;
> +
> + regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RCF);
> + regmap_write(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_CAL |
> + RPC_PHYCNT_STRTIM(6) | 0x260);
> +
> + return len;
> +out:
> + return rpc_spi_do_reset(rpc);
> +}
> +
> +static int rpc_spi_mem_dirmap_create(struct spi_mem_dirmap_desc *desc)
> +{
> + struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
> +
> + if (desc->info.offset + desc->info.length > U32_MAX)
> + return -ENOTSUPP;
> +
> + if (!rpc_spi_mem_supports_op(desc->mem, &desc->info.op_tmpl))
> + return -ENOTSUPP;
> +
> + if (!rpc->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;
> + rpc->xfer_dir = SPI_MEM_NO_DATA;
> +
> + list_for_each_entry(t, &msg->transfers, transfer_list) {
> + if (t->tx_buf) {
> + xfer[xferpos].tx_buf = t->tx_buf;
> + xfer[xferpos].tx_nbits = t->tx_nbits;
> + }
> +
> + if (t->rx_buf) {
> + xfer[xferpos].rx_buf = t->rx_buf;
> + xfer[xferpos].rx_nbits = t->rx_nbits;
> + }
> +
> + if (t->len) {
> + xfer[xferpos++].len = t->len;
> + rpc->totalxferlen += t->len;
> + }
> +
> + if (list_is_last(&t->transfer_list, &msg->transfers)) {
> + if (xferpos > 1 && t->rx_buf) {
> + rpc->xfer_dir = SPI_MEM_DATA_IN;
> + rpc->smcr = RPC_SMCR_SPIRE;
> + } else if (xferpos > 1 && t->tx_buf) {
> + rpc->xfer_dir = SPI_MEM_DATA_OUT;
> + rpc->smcr = RPC_SMCR_SPIWE;
> + }
> + }
> + }
> +
> + xfercnt = xferpos;
> + rpc->xferlen = xfer[--xferpos].len;
> + rpc->cmd = RPC_SMCMR_CMD(((u8 *)xfer[0].tx_buf)[0]);
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));
> + } 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));
> + }
> + 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));
It seems this SMENR pattern repeats itself throughout the driver, can
this be improved / deduplicated ?
> + } 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));
> + }
> + break;
> +
> + case 4:
> + if (xfer[2].len && xfer[2].tx_buf) {
> + rpc->smenr |= RPC_SMENR_DME;
> + rpc->dummy = RPC_SMDMCR_DMCYC(xfer[2].len);
> + }
> +
> + 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));
> + }
> + 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))
> + continue;
> + ret = rpc_spi_xfer_message(rpc, t);
> + if (ret)
> + goto out;
> + }
> +
> + msg->status = 0;
> + msg->actual_length = rpc->totalxferlen;
> +out:
> + spi_finalize_current_message(master);
> + return 0;
> +}
> +
> +static const struct regmap_range rpc_spi_volatile_ranges[] = {
> + regmap_reg_range(RPC_SMRDR0, RPC_SMRDR0),
> + regmap_reg_range(RPC_SMWDR0, RPC_SMWDR0),
Why is SMWDR volatile ?
> + regmap_reg_range(RPC_CMNSR, RPC_CMNSR),
> +};
> +
> +static const struct regmap_access_table rpc_spi_volatile_table = {
> + .yes_ranges = rpc_spi_volatile_ranges,
> + .n_yes_ranges = ARRAY_SIZE(rpc_spi_volatile_ranges),
> +};
> +
> +static const struct regmap_config rpc_spi_regmap_config = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 4,
> + .fast_io = true,
> + .max_register = RPC_WBUF + RPC_WBUF_SIZE,
> + .volatile_table = &rpc_spi_volatile_table,
> +};
> +
> +static int rpc_spi_probe(struct platform_device *pdev)
> +{
> + struct spi_master *master;
> + struct resource *res;
> + struct rpc_spi *rpc;
> + const struct regmap_config *regmap_config;
> + int ret;
> +
> + master = spi_alloc_master(&pdev->dev, sizeof(struct rpc_spi));
sizeof(*rpc)
> + if (!master)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, master);
> +
> + rpc = spi_master_get_devdata(master);
> +
> + master->dev.of_node = pdev->dev.of_node;
> +
> + rpc->clk_rpc = devm_clk_get(&pdev->dev, "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->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(rpc->base))
> + return PTR_ERR(rpc->base);
> +
> + regmap_config = &rpc_spi_regmap_config;
> + rpc->regmap = devm_regmap_init_mmio(&pdev->dev, rpc->base,
> + regmap_config);
> + if (IS_ERR(rpc->regmap)) {
> + dev_err(&pdev->dev, "failed to init regmap %ld for rpc-spi\n",
> + PTR_ERR(rpc->regmap));
> + return PTR_ERR(rpc->regmap);
> + }
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dirmap");
> + rpc->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;
> + }
> +
> + rpc->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> + if (IS_ERR(rpc->rstc))
> + return PTR_ERR(rpc->rstc);
> +
> + pm_runtime_enable(&pdev->dev);
> + master->auto_runtime_pm = true;
> +
> + master->num_chipselect = 1;
> + master->mem_ops = &rpc_spi_mem_ops;
> + master->transfer_one_message = rpc_spi_transfer_one_message;
> +
> + master->bits_per_word_mask = SPI_BPW_MASK(8);
> + master->mode_bits = SPI_CPOL | SPI_CPHA |
> + 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");
Knowing the return value would be useful.
> + goto err_put_master;
> + }
> + return 0;
> +
> +err_put_master:
> + spi_master_put(master);
> + pm_runtime_disable(&pdev->dev);
> +
> + return ret;
> +}
> +
> +static int rpc_spi_remove(struct platform_device *pdev)
> +{
> + struct spi_master *master = platform_get_drvdata(pdev);
> +
> + pm_runtime_disable(&pdev->dev);
> + spi_unregister_master(master);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id rpc_spi_of_ids[] = {
> + { .compatible = "renesas,r8a77995-rpc", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, rpc_spi_of_ids);
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int rpc_spi_suspend(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct spi_master *master = platform_get_drvdata(pdev);
> +
> + return spi_master_suspend(master);
Won't the SPI NOR lose state across suspend ? Is that a problem ?
> +}
> +
> +static int rpc_spi_resume(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct spi_master *master = platform_get_drvdata(pdev);
> +
> + return spi_master_resume(master);
> +}
> +
> +static SIMPLE_DEV_PM_OPS(rpc_spi_pm_ops, rpc_spi_suspend, rpc_spi_resume);
> +#define DEV_PM_OPS (&rpc_spi_pm_ops)
> +#else
> +#define DEV_PM_OPS NULL
> +#endif
> +
> +static struct platform_driver rpc_spi_driver = {
> + .probe = rpc_spi_probe,
> + .remove = rpc_spi_remove,
> + .driver = {
> + .name = "rpc-spi",
> + .of_match_table = rpc_spi_of_ids,
> + .pm = DEV_PM_OPS,
> + },
> +};
> +module_platform_driver(rpc_spi_driver);
> +
> +MODULE_AUTHOR("Mason Yang <[email protected]>");
> +MODULE_DESCRIPTION("Renesas R-Car Gen3 RPC SPI controller driver");
> +MODULE_LICENSE("GPL v2");
>
--
Best regards,
Marek Vasut
Hi Mason,
On Mon, Dec 3, 2018 at 10:19 AM Mason Yang <[email protected]> wrote:
> Add a driver for Renesas R-Car Gen3 RPC SPI controller.
>
> Signed-off-by: Mason Yang <[email protected]>
Thanks for your patch!
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -528,6 +528,12 @@ config SPI_RSPI
> help
> SPI driver for Renesas RSPI and QSPI blocks.
>
> +config SPI_RENESAS_RPC
> + tristate "Renesas R-Car Gen3 RPC SPI controller"
> + depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
So this driver is intended for SuperH SoCs, too?
If not, please drop the dependency.
> --- /dev/null
> +++ b/drivers/spi/spi-renesas-rpc.c
> +#ifdef CONFIG_RESET_CONTROLLER
> +static int rpc_spi_do_reset(struct rpc_spi *rpc)
What's the purpose of the reset routine?
Given the #ifdef, is it optional or required?
> +{
> + int i, ret;
> +
> + ret = reset_control_reset(rpc->rstc);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < LOOP_TIMEOUT; i++) {
> + ret = reset_control_status(rpc->rstc);
> + if (ret == 0)
> + return 0;
> + usleep_range(0, 1);
> + }
Why do you need this loop?
The delay in cpg_mssr_reset() should be sufficient.
> +
> + return -ETIMEDOUT;
> +}
> +#else
> +static int rpc_spi_do_reset(struct rpc_spi *rpc)
> +{
> + return -ETIMEDOUT;
> +}
> +#endif
> +static int rpc_spi_transfer_one_message(struct spi_master *master,
> + struct spi_message *msg)
> +{
> + struct rpc_spi *rpc = spi_master_get_devdata(master);
> + struct spi_transfer *t;
> + int ret;
> +
> + rpc_spi_transfer_setup(rpc, msg);
> +
> + list_for_each_entry(t, &msg->transfers, transfer_list) {
> + if (!list_is_last(&t->transfer_list, &msg->transfers))
> + continue;
> + ret = rpc_spi_xfer_message(rpc, t);
rpc_spi_xfer_message() sounds like a bad name to me, given it operates
on a transfer, not on a message.
> + if (ret)
> + goto out;
> + }
> +
> + msg->status = 0;
> + msg->actual_length = rpc->totalxferlen;
> +out:
> + spi_finalize_current_message(master);
> + return 0;
> +}
> +static int rpc_spi_probe(struct platform_device *pdev)
> +{
> + rpc->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> + if (IS_ERR(rpc->rstc))
> + return PTR_ERR(rpc->rstc);
This will return an error if CONFIG_RESET_CONTROLLER is not set, hence
the #ifdef above is moot.
> +
> + 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;
Is there any reason you cannot use the standard
spi_transfer_one_message, i.e. provide spi_controller.transfer_one()
instead of spi_controller.transfer_one_message()?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Mason,
On Wed, Dec 5, 2018 at 8:44 AM <[email protected]> wrote:
> > "Marek Vasut" <[email protected]>
> > 2018/12/05 上午 10:04
> > On 12/03/2018 10:18 AM, Mason Yang wrote:
> > > Add a driver for Renesas R-Car Gen3 RPC SPI controller.
> > >
> > > Signed-off-by: Mason Yang <[email protected]>
> > > +static u8 rpc_bits_xfer(u32 nbytes)
> > > +{
> > > + if (nbytes > 4)
> > > + nbytes = 4;
> >
> > Use clamp() ?
> >
>
> nbytes = clamp(nbytes, 1, 4);
>
> got many warnings, something like,
> ./include/linux/kernel.h:845:29: warning: comparison of distinct pointer types lacks a cast
You can either make the constants unsigned (1U and 4U), or
use clamp_t(u32, ...).
> > > + rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer
> > > + (op->data.nbytes)) | RPC_SMENR_SPIDB
> > > + (fls(op->data.buswidth >> 1));
> >
> > Drop parenthesis around fls()
>
> ?
> no way.
Please split the line before RPC_SMENR_SPIDB, and join the next line
with the parameters, so it becomes obvious the parentheses are needed
because RPC_SMENR_SPIDB() is a macro taking parameters.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 12/05/2018 10:11 AM, Geert Uytterhoeven wrote:
> Hi Mason,
>
> On Wed, Dec 5, 2018 at 8:44 AM <[email protected]> wrote:
>>> "Marek Vasut" <[email protected]>
>>> 2018/12/05 上午 10:04
>>> On 12/03/2018 10:18 AM, Mason Yang wrote:
>>>> Add a driver for Renesas R-Car Gen3 RPC SPI controller.
>>>>
>>>> Signed-off-by: Mason Yang <[email protected]>
>
>>>> +static u8 rpc_bits_xfer(u32 nbytes)
>>>> +{
>>>> + if (nbytes > 4)
>>>> + nbytes = 4;
>>>
>>> Use clamp() ?
>>>
>>
>> nbytes = clamp(nbytes, 1, 4);
>>
>> got many warnings, something like,
>> ./include/linux/kernel.h:845:29: warning: comparison of distinct pointer types lacks a cast
>
> You can either make the constants unsigned (1U and 4U), or
> use clamp_t(u32, ...).
>
>>>> + rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer
>>>> + (op->data.nbytes)) | RPC_SMENR_SPIDB
>>>> + (fls(op->data.buswidth >> 1));
>>>
>>> Drop parenthesis around fls()
>>
>> ?
>> no way.
>
> Please split the line before RPC_SMENR_SPIDB, and join the next line
> with the parameters, so it becomes obvious the parentheses are needed
> because RPC_SMENR_SPIDB() is a macro taking parameters.
Oh, I didn't notice that, right.
--
Best regards,
Marek Vasut
On 12/05/2018 08:44 AM, [email protected] wrote:
> Hi Marek,
Hi,
> thanks for your review.
>
>> "Marek Vasut" <[email protected]>
>> 2018/12/05 上午 10:04
>>
>> To
>>
>> "Mason Yang" <[email protected]>, [email protected], linux-
>> [email protected], [email protected],
>> [email protected], [email protected],
>> "Geert Uytterhoeven" <[email protected]>,
>>
>> cc
>>
>> [email protected], "Simon Horman" <[email protected]>,
>> [email protected]
>>
>> Subject
>>
>> Re: [PATCH v2 1/2] spi: Add Renesas R-Car Gen3 RPC SPI controller driver
>>
>> On 12/03/2018 10:18 AM, Mason Yang wrote:
>> > Add a driver for Renesas R-Car Gen3 RPC SPI controller.
>> >
>> > Signed-off-by: Mason Yang <[email protected]>
>>
>> What changed in this V2 ?
>>
>> [...]
>
> see some description in [PATH v2 0/2]
I don't see any V2: list there.
>> > +struct rpc_spi {
>> > + struct clk *clk_rpc;
>> > + void __iomem *base;
>> > + struct {
>> > + void __iomem *map;
>> > + dma_addr_t dma;
>> > + size_t size;
>> > + } linear;
>>
>> This is still here, see my review comments on the previous version.
>>
>> > + struct regmap *regmap;
>> > + u32 cur_speed_hz;
>> > + u32 cmd;
>> > + u32 addr;
>> > + u32 dummy;
>> > + u32 smcr;
>> > + u32 smenr;
>> > + u32 xferlen;
>> > + u32 totalxferlen;
>> > + enum spi_mem_data_dir xfer_dir;
>> > +#ifdef CONFIG_RESET_CONTROLLER
>> > + struct reset_control *rstc;
>> > +#endif
>> > +};
>> > +
>> > +static int rpc_spi_set_freq(struct rpc_spi *rpc, unsigned long freq)
>> > +{
>> > + int ret;
>> > +
>> > + if (rpc->cur_speed_hz == freq)
>> > + return 0;
>> > +
>> > + ret = clk_set_rate(rpc->clk_rpc, freq);
>> > + if (ret)
>> > + return ret;
>> > +
>> > + rpc->cur_speed_hz = freq;
>> > + return ret;
>> > +}
>> > +
>> > +static void rpc_spi_hw_init(struct rpc_spi *rpc)
>> > +{
>> > + /*
>> > + * NOTE: The 0x260 are undocumented bits, but they must be set.
>> > + * RPC_PHYCNT_STRTIM is strobe timing adjustment bit,
>> > + * 0x0 : the delay is biggest,
>> > + * 0x1 : the delay is 2nd biggest,
>> > + * 0x3 or 0x6 is a recommended value.
>> > + */
>>
>> Doesn't this vary by SoC ? I think H3 ES1.0 had different value here,
>> but I might be wrong.
>
> I check the Renesas bare-metal code, mini_monitor v4.01.
> It set 0x03 or 0x0 and I test them w/ 0x0, 0x3 and 0x6 are all OK.
Shouldn't this somehow use the soc_device_match() then and configure it
accordingly for various chips ? Like eg. the r8a7795-cpg-mssr driver does.
>> > + regmap_write(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_CAL |
>> > + RPC_PHYCNT_STRTIM(0x6) | 0x260);
>> > +
>> > + /*
>> > + * NOTE: The 0x31511144 and 0x431 are undocumented bits,
>> > + * but they must be set for RPC_PHYOFFSET1 & RPC_PHYOFFSET2.
>> > + */
>> > + regmap_write(rpc->regmap, RPC_PHYOFFSET1, 0x31511144);
>> > + regmap_write(rpc->regmap, RPC_PHYOFFSET2, 0x431);
>> > +
>> > + regmap_write(rpc->regmap, RPC_SSLDR, RPC_SSLDR_SPNDL(7) |
>> > + RPC_SSLDR_SLNDL(7) | RPC_SSLDR_SCKDL(7));
>> > +}
>> > +
>> > +#ifdef CONFIG_RESET_CONTROLLER
>>
>> Just make the driver depend on reset controller.
>
> ?
> please refer to
> https://github.com/torvalds/linux/blob/master/drivers/clk/renesas/renesas-cpg-mssr.c
>
> line 124 ~ 126
This seems like a stopgap measure for systems which do not have a reset
api compatible controller. Geert ?
>> > +static int rpc_spi_do_reset(struct rpc_spi *rpc)
>> > +{
>> > + int i, ret;
>> > +
>> > + ret = reset_control_reset(rpc->rstc);
>> > + if (ret)
>> > + return ret;
>> > +
>> > + for (i = 0; i < LOOP_TIMEOUT; i++) {
>> > + ret = reset_control_status(rpc->rstc);
>> > + if (ret == 0)
>> > + return 0;
>> > + usleep_range(0, 1);
>> > + }
>> > +
>> > + return -ETIMEDOUT;
>> > +}
>> > +#else
>> > +static int rpc_spi_do_reset(struct rpc_spi *rpc)
>> > +{
>> > + return -ETIMEDOUT;
>> > +}
>> > +#endif
>> > +
>> > +static int wait_msg_xfer_end(struct rpc_spi *rpc)
>> > +{
>> > + u32 sts;
>> > +
>> > + return regmap_read_poll_timeout(rpc->regmap, RPC_CMNSR, sts,
>> > + sts & RPC_CMNSR_TEND, 0, USEC_PER_SEC);
>> > +}
>> > +
>> > +static u8 rpc_bits_xfer(u32 nbytes)
>> > +{
>> > + if (nbytes > 4)
>> > + nbytes = 4;
>>
>> Use clamp() ?
>>
>
> nbytes = clamp(nbytes, 1, 4);
>
> got many warnings, something like,
> ./include/linux/kernel.h:845:29: warning: comparison of distinct pointer
> types lacks a cast
Geert already replied.
>> > +
>> > + return GENMASK(3, 4 - nbytes);
>>
>> Nice ;-)
>>
>> > +}
>> > +
>> > +static int rpc_spi_io_xfer(struct rpc_spi *rpc,
>> > + const void *tx_buf, void *rx_buf)
>> > +{
>> > + u32 smenr, smcr, data, pos = 0;
>> > + int ret = 0;
>> > +
>> > + regmap_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD | RPC_CMNCR_SFDE |
>> > + RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
>> > + RPC_CMNCR_BSZ(0));
>> > + regmap_write(rpc->regmap, RPC_SMDRENR, 0x0);
>> > + regmap_write(rpc->regmap, RPC_SMCMR, rpc->cmd);
>> > + regmap_write(rpc->regmap, RPC_SMDMCR, rpc->dummy);
>> > + regmap_write(rpc->regmap, RPC_SMADR, rpc->addr);
>> > +
>> > + if (tx_buf) {
>> > + smenr = rpc->smenr;
>> > +
>> > + while (pos < rpc->xferlen) {
>> > + u32 nbytes = rpc->xferlen - pos;
>> > +
>> > + regmap_write(rpc->regmap, RPC_SMWDR0,
>> > + *(u32 *)(tx_buf + pos));
>>
>> *(u32 *) cast is probably not needed , fix casts globally.
>
> It must have it!
Why ?
>> > + if (nbytes > 4) {
>> > + nbytes = 4;
>> > + smcr = rpc->smcr |
>> > + RPC_SMCR_SPIE | RPC_SMCR_SSLKP;
>> > + } else {
>> > + smcr = rpc->smcr | RPC_SMCR_SPIE;
>> > + }
>> > +
>> > + regmap_write(rpc->regmap, RPC_SMENR, smenr);
>> > + regmap_write(rpc->regmap, RPC_SMCR, smcr);
>> > + ret = wait_msg_xfer_end(rpc);
>> > + if (ret)
>> > + goto out;
>> > +
>> > + pos += nbytes;
>> > + smenr = rpc->smenr & ~RPC_SMENR_CDE &
>> > + ~RPC_SMENR_ADE(0xf);
>> > + }
>> > + } else if (rx_buf) {
>> > + while (pos < rpc->xferlen) {
>> > + u32 nbytes = rpc->xferlen - pos;
>> > +
>> > + if (nbytes > 4)
>> > + nbytes = 4;
>> > +
>> > + regmap_write(rpc->regmap, RPC_SMENR, rpc->smenr);
>> > + regmap_write(rpc->regmap, RPC_SMCR,
>> > + rpc->smcr | RPC_SMCR_SPIE);
>> > + ret = wait_msg_xfer_end(rpc);
>> > + if (ret)
>> > + goto out;
>> > +
>> > + regmap_read(rpc->regmap, RPC_SMRDR0, &data);
>> > + memcpy_fromio(rx_buf + pos, (void *)&data, nbytes);
>> > + pos += nbytes;
>> > + regmap_write(rpc->regmap, RPC_SMCMR, rpc->cmd);
>> > + regmap_write(rpc->regmap, RPC_SMDMCR, rpc->dummy);
>> > + regmap_write(rpc->regmap, RPC_SMADR, rpc->addr + pos);
>> > + }
>> > + } else {
>> > + regmap_write(rpc->regmap, RPC_SMENR, rpc->smenr);
>> > + regmap_write(rpc->regmap, RPC_SMCR, rpc->smcr | RPC_SMCR_SPIE);
>> > + ret = wait_msg_xfer_end(rpc);
>> > + if (ret)
>> > + goto out;
>> > + }
>> > +
>> > + return ret;
>> > +out:
>> > + return rpc_spi_do_reset(rpc);
>> > +}
>> > +
>> > +static void rpc_spi_mem_set_prep_op_cfg(struct spi_device *spi,
>> > + const struct spi_mem_op *op,
>> > + u64 *offs, size_t *len)
>> > +{
>> > + struct rpc_spi *rpc = spi_master_get_devdata(spi->master);
>> > +
>> > + rpc->cmd = RPC_SMCMR_CMD(op->cmd.opcode);
>> > + rpc->smenr = RPC_SMENR_CDE |
>> > + RPC_SMENR_CDB(fls(op->cmd.buswidth >> 1));
>> > + rpc->totalxferlen = 1;
>> > + rpc->xfer_dir = SPI_MEM_NO_DATA;
>> > + rpc->xferlen = 0;
>> > + rpc->addr = 0;
>> > +
>> > + if (op->addr.nbytes) {
>> > + rpc->smenr |= RPC_SMENR_ADB(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)) {
>> > + if (op->data.dir == SPI_MEM_DATA_IN) {
>> > + rpc->smcr = RPC_SMCR_SPIRE;
>> > + rpc->xfer_dir = SPI_MEM_DATA_IN;
>> > + } else if (op->data.dir == SPI_MEM_DATA_OUT) {
>> > + rpc->smcr = RPC_SMCR_SPIWE;
>> > + rpc->xfer_dir = SPI_MEM_DATA_OUT;
>> > + }
>> > +
>> > + if (offs && len) {
>> > + rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer
>> > + (*(u32 *)len)) | RPC_SMENR_SPIDB
>> > + (fls(op->data.buswidth >> 1));
>>
>> Fix the *(u32 *)
>>
>> > + rpc->xferlen = *(u32 *)len;
>> > + rpc->totalxferlen += *(u32 *)len;
>> > + } else {
>> > + rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer
>> > + (op->data.nbytes)) | RPC_SMENR_SPIDB
>> > + (fls(op->data.buswidth >> 1));
>>
>> Drop parenthesis around fls()
>
> ?
> no way.
I would really appreciate it if you could explain things instead.
Geert already did so, by pointing out this is a confusing code
formatting problem and how it should be fixed, so no need to repeat
that. But I hope you understand how that sort of explanation is far more
valuable than "no way" kind of reply.
>> > +
>> > + 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)
>>
>> Merge this into previous conditional statement.
>>
>> > + 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);
>> > +
>> > + regmap_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_SFDE |
>> > + RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
>> > + RPC_CMNCR_BSZ(0));
>> > + regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RBURST(0x1f) |
>> > + RPC_DRCR_RBE);
>> > + regmap_write(rpc->regmap, RPC_DRCMR, rpc->cmd);
>> > + regmap_write(rpc->regmap, RPC_DREAR, RPC_DREAR_EAC);
>> > + regmap_write(rpc->regmap, RPC_DROPR, 0);
>> > + regmap_write(rpc->regmap, RPC_DRENR, rpc->smenr);
>> > + regmap_write(rpc->regmap, RPC_DRDMCR, rpc->dummy);
>> > + regmap_write(rpc->regmap, RPC_DRDRENR, 0);
>> > +
>> > + memcpy_fromio(buf, rpc->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 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);
>> > +
>> > + regmap_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD | RPC_CMNCR_SFDE |
>> > + RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
>> > + RPC_CMNCR_BSZ(0));
>> > + regmap_write(rpc->regmap, RPC_SMDRENR, 0);
>> > + regmap_write(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_CAL | 0x260 |
>> > + RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF);
>> > +
>> > + memcpy_toio(rpc->base + RPC_WBUF, buf, len);
>> > +
>> > + regmap_write(rpc->regmap, RPC_SMCMR, rpc->cmd);
>> > + regmap_write(rpc->regmap, RPC_SMADR, offs);
>> > + regmap_write(rpc->regmap, RPC_SMENR, rpc->smenr);
>> > + regmap_write(rpc->regmap, RPC_SMCR, rpc->smcr | RPC_SMCR_SPIE);
>> > + ret = wait_msg_xfer_end(rpc);
>> > + if (ret)
>> > + goto out;
>> > +
>> > + regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RCF);
>> > + regmap_write(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_CAL |
>> > + RPC_PHYCNT_STRTIM(6) | 0x260);
>> > +
>> > + return len;
>> > +out:
>> > + return rpc_spi_do_reset(rpc);
>> > +}
>> > +
>> > +static int rpc_spi_mem_dirmap_create(struct spi_mem_dirmap_desc *desc)
>> > +{
>> > + struct rpc_spi *rpc =
> spi_master_get_devdata(desc->mem->spi->master);
>> > +
>> > + if (desc->info.offset + desc->info.length > U32_MAX)
>> > + return -ENOTSUPP;
>> > +
>> > + if (!rpc_spi_mem_supports_op(desc->mem, &desc->info.op_tmpl))
>> > + return -ENOTSUPP;
>> > +
>> > + if (!rpc->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;
>> > + rpc->xfer_dir = SPI_MEM_NO_DATA;
>> > +
>> > + list_for_each_entry(t, &msg->transfers, transfer_list) {
>> > + if (t->tx_buf) {
>> > + xfer[xferpos].tx_buf = t->tx_buf;
>> > + xfer[xferpos].tx_nbits = t->tx_nbits;
>> > + }
>> > +
>> > + if (t->rx_buf) {
>> > + xfer[xferpos].rx_buf = t->rx_buf;
>> > + xfer[xferpos].rx_nbits = t->rx_nbits;
>> > + }
>> > +
>> > + if (t->len) {
>> > + xfer[xferpos++].len = t->len;
>> > + rpc->totalxferlen += t->len;
>> > + }
>> > +
>> > + if (list_is_last(&t->transfer_list, &msg->transfers)) {
>> > + if (xferpos > 1 && t->rx_buf) {
>> > + rpc->xfer_dir = SPI_MEM_DATA_IN;
>> > + rpc->smcr = RPC_SMCR_SPIRE;
>> > + } else if (xferpos > 1 && t->tx_buf) {
>> > + rpc->xfer_dir = SPI_MEM_DATA_OUT;
>> > + rpc->smcr = RPC_SMCR_SPIWE;
>> > + }
>> > + }
>> > + }
>> > +
>> > + xfercnt = xferpos;
>> > + rpc->xferlen = xfer[--xferpos].len;
>> > + rpc->cmd = RPC_SMCMR_CMD(((u8 *)xfer[0].tx_buf)[0]);
>>
>> Is the cast needed ?
>
> yes!
Why ?
>> > + 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));
>> > + } 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));
>> > + }
>> > + 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));
>>
>> It seems this SMENR pattern repeats itself throughout the driver, can
>> this be improved / deduplicated ?
>
> It seems no way!
Why ?
>> > + } 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));
>> > + }
>> > + break;
>> > +
>> > + case 4:
>> > + if (xfer[2].len && xfer[2].tx_buf) {
>> > + rpc->smenr |= RPC_SMENR_DME;
>> > + rpc->dummy = RPC_SMDMCR_DMCYC(xfer[2].len);
>> > + }
>> > +
>> > + 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));
>> > + }
>> > + 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))
>> > + continue;
>> > + ret = rpc_spi_xfer_message(rpc, t);
>> > + if (ret)
>> > + goto out;
>> > + }
>> > +
>> > + msg->status = 0;
>> > + msg->actual_length = rpc->totalxferlen;
>> > +out:
>> > + spi_finalize_current_message(master);
>> > + return 0;
>> > +}
>> > +
>> > +static const struct regmap_range rpc_spi_volatile_ranges[] = {
>> > + regmap_reg_range(RPC_SMRDR0, RPC_SMRDR0),
>> > + regmap_reg_range(RPC_SMWDR0, RPC_SMWDR0),
>>
>> Why is SMWDR volatile ?
>
> No matter is write-back or write-through.
Oh, do you want to assure the SMWDR value is always written directly to
the hardware ?
btw. I think SMRDR should be precious ?
>> > + regmap_reg_range(RPC_CMNSR, RPC_CMNSR),
>> > +};
>> > +
>> > +static const struct regmap_access_table rpc_spi_volatile_table = {
>> > + .yes_ranges = rpc_spi_volatile_ranges,
>> > + .n_yes_ranges = ARRAY_SIZE(rpc_spi_volatile_ranges),
>> > +};
>> > +
>> > +static const struct regmap_config rpc_spi_regmap_config = {
>> > + .reg_bits = 32,
>> > + .val_bits = 32,
>> > + .reg_stride = 4,
>> > + .fast_io = true,
>> > + .max_register = RPC_WBUF + RPC_WBUF_SIZE,
>> > + .volatile_table = &rpc_spi_volatile_table,
>> > +};
>> > +
>> > +static int rpc_spi_probe(struct platform_device *pdev)
>> > +{
>> > + struct spi_master *master;
>> > + struct resource *res;
>> > + struct rpc_spi *rpc;
>> > + const struct regmap_config *regmap_config;
>> > + int ret;
>> > +
>> > + master = spi_alloc_master(&pdev->dev, sizeof(struct rpc_spi));
>>
>> sizeof(*rpc)
>>
>> > + if (!master)
>> > + return -ENOMEM;
>> > +
>> > + platform_set_drvdata(pdev, master);
>> > +
>> > + rpc = spi_master_get_devdata(master);
>> > +
>> > + master->dev.of_node = pdev->dev.of_node;
>> > +
>> > + rpc->clk_rpc = devm_clk_get(&pdev->dev, "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->base = devm_ioremap_resource(&pdev->dev, res);
>> > + if (IS_ERR(rpc->base))
>> > + return PTR_ERR(rpc->base);
>> > +
>> > + regmap_config = &rpc_spi_regmap_config;
>> > + rpc->regmap = devm_regmap_init_mmio(&pdev->dev, rpc->base,
>> > + regmap_config);
>> > + if (IS_ERR(rpc->regmap)) {
>> > + dev_err(&pdev->dev, "failed to init regmap %ld for rpc-spi\n",
>> > + PTR_ERR(rpc->regmap));
>> > + return PTR_ERR(rpc->regmap);
>> > + }
>> > +
>> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dirmap");
>> > + rpc->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;
>> > + }
>> > +
>> > + rpc->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
>> > + if (IS_ERR(rpc->rstc))
>> > + return PTR_ERR(rpc->rstc);
>> > +
>> > + pm_runtime_enable(&pdev->dev);
>> > + master->auto_runtime_pm = true;
>> > +
>> > + master->num_chipselect = 1;
>> > + master->mem_ops = &rpc_spi_mem_ops;
>> > + master->transfer_one_message = rpc_spi_transfer_one_message;
>> > +
>> > + master->bits_per_word_mask = SPI_BPW_MASK(8);
>> > + master->mode_bits = SPI_CPOL | SPI_CPHA |
>> > + 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");
>>
>> Knowing the return value would be useful.
>>
>> > + goto err_put_master;
>> > + }
>> > + return 0;
>> > +
>> > +err_put_master:
>> > + spi_master_put(master);
>> > + pm_runtime_disable(&pdev->dev);
>> > +
>> > + return ret;
>> > +}
>> > +
>> > +static int rpc_spi_remove(struct platform_device *pdev)
>> > +{
>> > + struct spi_master *master = platform_get_drvdata(pdev);
>> > +
>> > + pm_runtime_disable(&pdev->dev);
>> > + spi_unregister_master(master);
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +static const struct of_device_id rpc_spi_of_ids[] = {
>> > + { .compatible = "renesas,r8a77995-rpc", },
>> > + { /* sentinel */ }
>> > +};
>> > +MODULE_DEVICE_TABLE(of, rpc_spi_of_ids);
>> > +
>> > +#ifdef CONFIG_PM_SLEEP
>> > +static int rpc_spi_suspend(struct device *dev)
>> > +{
>> > + struct platform_device *pdev = to_platform_device(dev);
>> > + struct spi_master *master = platform_get_drvdata(pdev);
>> > +
>> > + return spi_master_suspend(master);
>>
>> Won't the SPI NOR lose state across suspend ? Is that a problem ?
>
> I don't think so.
> Because when the device is not in operation and CS# is high,
> it is put in stand-by mode.
Is the power to the SPI NOR retained ?
> 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.
>
> =====================================================================
--
Best regards,
Marek Vasut
On 12/05/2018 09:39 AM, [email protected] wrote:
> Hi Marek,
Hi,
>> "Marek Vasut" <[email protected]>
>> 2018/12/05 ?W?? 10:04
>>
>> To
>>
>> "Mason Yang" <[email protected]>, [email protected], linux-
>> [email protected], [email protected],
>> [email protected], [email protected],
>> "Geert Uytterhoeven" <[email protected]>,
>>
>> cc
>>
>> [email protected], "Simon Horman" <[email protected]>,
>> [email protected]
>>
>> Subject
>>
>> Re: [PATCH v2 2/2] dt-binding: spi: Document Renesas R-Car Gen3 RPC
>> controller bindings
>>
>> On 12/03/2018 10:18 AM, Mason Yang wrote:
>> > Document the bindings used by the Renesas R-Car Gen3 RPC SPI controller.
>>
>> RPC is SPI and HF controller, it is not a pure SPI controller.
>>
>> How does this deal with the HF part ? Keep in mind the bindings are ABI
>> and it will be difficult to redo them later.
>
> After checking HF datasheet,
> 512_MBIT_256_MBIT_128_MBIT_HYPERFLASH_FAMILY_Datasheet.pdf.
>
>
>
> From my point of view,
> HyperFlash is a kind of standard NOR Flash with high-speed SPI interface.
> But I might be wrong.
It behaves as a Parallel CFI NOR internally, and so it fits and works
with the CFI NOR drivers (see the U-Boot driver), except the interface
between the HF controller and the chip is custom. It's not SPI per-se.
--
Best regards,
Marek Vasut
Hi Marek,
On Wed, Dec 5, 2018 at 1:57 PM Marek Vasut <[email protected]> wrote:
> On 12/05/2018 08:44 AM, [email protected] wrote:
> >> "Marek Vasut" <[email protected]>
> >> 2018/12/05 上午 10:04
> >> On 12/03/2018 10:18 AM, Mason Yang wrote:
> >> > Add a driver for Renesas R-Car Gen3 RPC SPI controller.
> >> >
> >> > Signed-off-by: Mason Yang <[email protected]>
> >> > +static void rpc_spi_hw_init(struct rpc_spi *rpc)
> >> > +{
> >> > + /*
> >> > + * NOTE: The 0x260 are undocumented bits, but they must be set.
> >> > + * RPC_PHYCNT_STRTIM is strobe timing adjustment bit,
> >> > + * 0x0 : the delay is biggest,
> >> > + * 0x1 : the delay is 2nd biggest,
> >> > + * 0x3 or 0x6 is a recommended value.
> >> > + */
> >>
> >> Doesn't this vary by SoC ? I think H3 ES1.0 had different value here,
> >> but I might be wrong.
> >
> > I check the Renesas bare-metal code, mini_monitor v4.01.
> > It set 0x03 or 0x0 and I test them w/ 0x0, 0x3 and 0x6 are all OK.
>
> Shouldn't this somehow use the soc_device_match() then and configure it
> accordingly for various chips ? Like eg. the r8a7795-cpg-mssr driver does.
Please don't use soc_device_match() for per-SoC configuration, if
you already have of_device_id.data.
BTW, this drivers support r8a7795 only (for now), as per the compatible
value.
> >> > +#ifdef CONFIG_RESET_CONTROLLER
> >>
> >> Just make the driver depend on reset controller.
> >
> > ?
> > please refer to
> > https://github.com/torvalds/linux/blob/master/drivers/clk/renesas/renesas-cpg-mssr.c
> >
> > line 124 ~ 126
>
> This seems like a stopgap measure for systems which do not have a reset
> api compatible controller. Geert ?
So far CONFIG_RESET_CONTROLLER is optional.
> >> > +static int rpc_spi_io_xfer(struct rpc_spi *rpc,
> >> > + const void *tx_buf, void *rx_buf)
> >> > +{
> >> > + u32 smenr, smcr, data, pos = 0;
> >> > + int ret = 0;
> >> > +
> >> > + regmap_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD | RPC_CMNCR_SFDE |
> >> > + RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
> >> > + RPC_CMNCR_BSZ(0));
> >> > + regmap_write(rpc->regmap, RPC_SMDRENR, 0x0);
> >> > + regmap_write(rpc->regmap, RPC_SMCMR, rpc->cmd);
> >> > + regmap_write(rpc->regmap, RPC_SMDMCR, rpc->dummy);
> >> > + regmap_write(rpc->regmap, RPC_SMADR, rpc->addr);
> >> > +
> >> > + if (tx_buf) {
> >> > + smenr = rpc->smenr;
> >> > +
> >> > + while (pos < rpc->xferlen) {
> >> > + u32 nbytes = rpc->xferlen - pos;
> >> > +
> >> > + regmap_write(rpc->regmap, RPC_SMWDR0,
> >> > + *(u32 *)(tx_buf + pos));
> >>
> >> *(u32 *) cast is probably not needed , fix casts globally.
> >
> > It must have it!
>
> Why ?
Else you get a compiler warning, as tx_bug is void *.
> >> > +#ifdef CONFIG_PM_SLEEP
> >> > +static int rpc_spi_suspend(struct device *dev)
> >> > +{
> >> > + struct platform_device *pdev = to_platform_device(dev);
> >> > + struct spi_master *master = platform_get_drvdata(pdev);
> >> > +
> >> > + return spi_master_suspend(master);
> >>
> >> Won't the SPI NOR lose state across suspend ? Is that a problem ?
> >
> > I don't think so.
> > Because when the device is not in operation and CS# is high,
> > it is put in stand-by mode.
>
> Is the power to the SPI NOR retained ?
Not if PSCI system suspend turns of power to the SoC.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 12/05/2018 02:15 PM, Geert Uytterhoeven wrote:
> Hi Marek,
Hi,
> On Wed, Dec 5, 2018 at 1:57 PM Marek Vasut <[email protected]> wrote:
>> On 12/05/2018 08:44 AM, [email protected] wrote:
>>>> "Marek Vasut" <[email protected]>
>>>> 2018/12/05 上午 10:04
>>>> On 12/03/2018 10:18 AM, Mason Yang wrote:
>>>>> Add a driver for Renesas R-Car Gen3 RPC SPI controller.
>>>>>
>>>>> Signed-off-by: Mason Yang <[email protected]>
>
>>>>> +static void rpc_spi_hw_init(struct rpc_spi *rpc)
>>>>> +{
>>>>> + /*
>>>>> + * NOTE: The 0x260 are undocumented bits, but they must be set.
>>>>> + * RPC_PHYCNT_STRTIM is strobe timing adjustment bit,
>>>>> + * 0x0 : the delay is biggest,
>>>>> + * 0x1 : the delay is 2nd biggest,
>>>>> + * 0x3 or 0x6 is a recommended value.
>>>>> + */
>>>>
>>>> Doesn't this vary by SoC ? I think H3 ES1.0 had different value here,
>>>> but I might be wrong.
>>>
>>> I check the Renesas bare-metal code, mini_monitor v4.01.
>>> It set 0x03 or 0x0 and I test them w/ 0x0, 0x3 and 0x6 are all OK.
>>
>> Shouldn't this somehow use the soc_device_match() then and configure it
>> accordingly for various chips ? Like eg. the r8a7795-cpg-mssr driver does.
>
> Please don't use soc_device_match() for per-SoC configuration, if
> you already have of_device_id.data.
I mean, the value is different on H3 ES1 and ES2 iirc, that's what
soc_device_match() is for, right ?
> BTW, this drivers support r8a7795 only (for now), as per the compatible
> value.
77995
>>>>> +#ifdef CONFIG_RESET_CONTROLLER
>>>>
>>>> Just make the driver depend on reset controller.
>>>
>>> ?
>>> please refer to
>>> https://github.com/torvalds/linux/blob/master/drivers/clk/renesas/renesas-cpg-mssr.c
>>>
>>> line 124 ~ 126
>>
>> This seems like a stopgap measure for systems which do not have a reset
>> api compatible controller. Geert ?
>
> So far CONFIG_RESET_CONTROLLER is optional.
My understanding is that for this IP, it can well be mandatory, since
all the chips have a reset wired to the IP internally.
>>>>> +static int rpc_spi_io_xfer(struct rpc_spi *rpc,
>>>>> + const void *tx_buf, void *rx_buf)
>>>>> +{
>>>>> + u32 smenr, smcr, data, pos = 0;
>>>>> + int ret = 0;
>>>>> +
>>>>> + regmap_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD | RPC_CMNCR_SFDE |
>>>>> + RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
>>>>> + RPC_CMNCR_BSZ(0));
>>>>> + regmap_write(rpc->regmap, RPC_SMDRENR, 0x0);
>>>>> + regmap_write(rpc->regmap, RPC_SMCMR, rpc->cmd);
>>>>> + regmap_write(rpc->regmap, RPC_SMDMCR, rpc->dummy);
>>>>> + regmap_write(rpc->regmap, RPC_SMADR, rpc->addr);
>>>>> +
>>>>> + if (tx_buf) {
>>>>> + smenr = rpc->smenr;
>>>>> +
>>>>> + while (pos < rpc->xferlen) {
>>>>> + u32 nbytes = rpc->xferlen - pos;
>>>>> +
>>>>> + regmap_write(rpc->regmap, RPC_SMWDR0,
>>>>> + *(u32 *)(tx_buf + pos));
>>>>
>>>> *(u32 *) cast is probably not needed , fix casts globally.
>>>
>>> It must have it!
>>
>> Why ?
>
> Else you get a compiler warning, as tx_bug is void *.
Don't you need some get_unaligned() in that case ? txbuf+pos can well be
unaligned if it's void * .
>>>>> +#ifdef CONFIG_PM_SLEEP
>>>>> +static int rpc_spi_suspend(struct device *dev)
>>>>> +{
>>>>> + struct platform_device *pdev = to_platform_device(dev);
>>>>> + struct spi_master *master = platform_get_drvdata(pdev);
>>>>> +
>>>>> + return spi_master_suspend(master);
>>>>
>>>> Won't the SPI NOR lose state across suspend ? Is that a problem ?
>>>
>>> I don't think so.
>>> Because when the device is not in operation and CS# is high,
>>> it is put in stand-by mode.
>>
>> Is the power to the SPI NOR retained ?
>
> Not if PSCI system suspend turns of power to the SoC.
And is that a problem ?
--
Best regards,
Marek Vasut
Hi Marek,
On Wed, Dec 5, 2018 at 2:25 PM Marek Vasut <[email protected]> wrote:
> On 12/05/2018 02:15 PM, Geert Uytterhoeven wrote:
> > On Wed, Dec 5, 2018 at 1:57 PM Marek Vasut <[email protected]> wrote:
> >> On 12/05/2018 08:44 AM, [email protected] wrote:
> >>>> "Marek Vasut" <[email protected]>
> >>>> 2018/12/05 上午 10:04
> >>>> On 12/03/2018 10:18 AM, Mason Yang wrote:
> >>>>> Add a driver for Renesas R-Car Gen3 RPC SPI controller.
> >>>>>
> >>>>> Signed-off-by: Mason Yang <[email protected]>
> >
> >>>>> +static void rpc_spi_hw_init(struct rpc_spi *rpc)
> >>>>> +{
> >>>>> + /*
> >>>>> + * NOTE: The 0x260 are undocumented bits, but they must be set.
> >>>>> + * RPC_PHYCNT_STRTIM is strobe timing adjustment bit,
> >>>>> + * 0x0 : the delay is biggest,
> >>>>> + * 0x1 : the delay is 2nd biggest,
> >>>>> + * 0x3 or 0x6 is a recommended value.
> >>>>> + */
> >>>>
> >>>> Doesn't this vary by SoC ? I think H3 ES1.0 had different value here,
> >>>> but I might be wrong.
> >>>
> >>> I check the Renesas bare-metal code, mini_monitor v4.01.
> >>> It set 0x03 or 0x0 and I test them w/ 0x0, 0x3 and 0x6 are all OK.
> >>
> >> Shouldn't this somehow use the soc_device_match() then and configure it
> >> accordingly for various chips ? Like eg. the r8a7795-cpg-mssr driver does.
> >
> > Please don't use soc_device_match() for per-SoC configuration, if
> > you already have of_device_id.data.
>
> I mean, the value is different on H3 ES1 and ES2 iirc, that's what
> soc_device_match() is for, right ?
Oh, it differs between revisions, too?
Yes, in that case you need soc_device_match().
> > BTW, this drivers support r8a7795 only (for now), as per the compatible
> > value.
>
> 77995
Sorry, typo on my side. So H3 is not yet supported ;-)
> >>>>> +#ifdef CONFIG_RESET_CONTROLLER
> >>>>
> >>>> Just make the driver depend on reset controller.
> >>>
> >>> ?
> >>> please refer to
> >>> https://github.com/torvalds/linux/blob/master/drivers/clk/renesas/renesas-cpg-mssr.c
> >>>
> >>> line 124 ~ 126
> >>
> >> This seems like a stopgap measure for systems which do not have a reset
> >> api compatible controller. Geert ?
> >
> > So far CONFIG_RESET_CONTROLLER is optional.
>
> My understanding is that for this IP, it can well be mandatory, since
> all the chips have a reset wired to the IP internally.
That's what I was trying to find out, hence my question about the purpose.
> >>>>> + regmap_write(rpc->regmap, RPC_SMWDR0,
> >>>>> + *(u32 *)(tx_buf + pos));
> >>>>
> >>>> *(u32 *) cast is probably not needed , fix casts globally.
> >>>
> >>> It must have it!
> >>
> >> Why ?
> >
> > Else you get a compiler warning, as tx_bug is void *.
>
> Don't you need some get_unaligned() in that case ? txbuf+pos can well be
> unaligned if it's void * .
True, but IIRC, arm64 can handle that, right?
Don't know about SuperH.
> >>>>> +#ifdef CONFIG_PM_SLEEP
> >>>>> +static int rpc_spi_suspend(struct device *dev)
> >>>>> +{
> >>>>> + struct platform_device *pdev = to_platform_device(dev);
> >>>>> + struct spi_master *master = platform_get_drvdata(pdev);
> >>>>> +
> >>>>> + return spi_master_suspend(master);
> >>>>
> >>>> Won't the SPI NOR lose state across suspend ? Is that a problem ?
> >>>
> >>> I don't think so.
> >>> Because when the device is not in operation and CS# is high,
> >>> it is put in stand-by mode.
> >>
> >> Is the power to the SPI NOR retained ?
> >
> > Not if PSCI system suspend turns of power to the SoC.
>
> And is that a problem ?
Good question!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 12/05/2018 02:31 PM, Geert Uytterhoeven wrote:
> Hi Marek,
Hi,
> On Wed, Dec 5, 2018 at 2:25 PM Marek Vasut <[email protected]> wrote:
>> On 12/05/2018 02:15 PM, Geert Uytterhoeven wrote:
>>> On Wed, Dec 5, 2018 at 1:57 PM Marek Vasut <[email protected]> wrote:
>>>> On 12/05/2018 08:44 AM, [email protected] wrote:
>>>>>> "Marek Vasut" <[email protected]>
>>>>>> 2018/12/05 上午 10:04
>>>>>> On 12/03/2018 10:18 AM, Mason Yang wrote:
>>>>>>> Add a driver for Renesas R-Car Gen3 RPC SPI controller.
>>>>>>>
>>>>>>> Signed-off-by: Mason Yang <[email protected]>
>>>
>>>>>>> +static void rpc_spi_hw_init(struct rpc_spi *rpc)
>>>>>>> +{
>>>>>>> + /*
>>>>>>> + * NOTE: The 0x260 are undocumented bits, but they must be set.
>>>>>>> + * RPC_PHYCNT_STRTIM is strobe timing adjustment bit,
>>>>>>> + * 0x0 : the delay is biggest,
>>>>>>> + * 0x1 : the delay is 2nd biggest,
>>>>>>> + * 0x3 or 0x6 is a recommended value.
>>>>>>> + */
>>>>>>
>>>>>> Doesn't this vary by SoC ? I think H3 ES1.0 had different value here,
>>>>>> but I might be wrong.
>>>>>
>>>>> I check the Renesas bare-metal code, mini_monitor v4.01.
>>>>> It set 0x03 or 0x0 and I test them w/ 0x0, 0x3 and 0x6 are all OK.
>>>>
>>>> Shouldn't this somehow use the soc_device_match() then and configure it
>>>> accordingly for various chips ? Like eg. the r8a7795-cpg-mssr driver does.
>>>
>>> Please don't use soc_device_match() for per-SoC configuration, if
>>> you already have of_device_id.data.
>>
>> I mean, the value is different on H3 ES1 and ES2 iirc, that's what
>> soc_device_match() is for, right ?
>
> Oh, it differs between revisions, too?
> Yes, in that case you need soc_device_match().
>
>>> BTW, this drivers support r8a7795 only (for now), as per the compatible
>>> value.
>>
>> 77995
>
> Sorry, typo on my side. So H3 is not yet supported ;-)
It's coming the minute this lands mainline, so we should make it easy to
add.
>>>>>>> +#ifdef CONFIG_RESET_CONTROLLER
>>>>>>
>>>>>> Just make the driver depend on reset controller.
>>>>>
>>>>> ?
>>>>> please refer to
>>>>> https://github.com/torvalds/linux/blob/master/drivers/clk/renesas/renesas-cpg-mssr.c
>>>>>
>>>>> line 124 ~ 126
>>>>
>>>> This seems like a stopgap measure for systems which do not have a reset
>>>> api compatible controller. Geert ?
>>>
>>> So far CONFIG_RESET_CONTROLLER is optional.
>>
>> My understanding is that for this IP, it can well be mandatory, since
>> all the chips have a reset wired to the IP internally.
>
> That's what I was trying to find out, hence my question about the purpose.
>
>>>>>>> + regmap_write(rpc->regmap, RPC_SMWDR0,
>>>>>>> + *(u32 *)(tx_buf + pos));
>>>>>>
>>>>>> *(u32 *) cast is probably not needed , fix casts globally.
>>>>>
>>>>> It must have it!
>>>>
>>>> Why ?
>>>
>>> Else you get a compiler warning, as tx_bug is void *.
>>
>> Don't you need some get_unaligned() in that case ? txbuf+pos can well be
>> unaligned if it's void * .
>
> True, but IIRC, arm64 can handle that, right?
> Don't know about SuperH.
Oh, that's right, there are SH systems with RPC. Right.
>>>>>>> +#ifdef CONFIG_PM_SLEEP
>>>>>>> +static int rpc_spi_suspend(struct device *dev)
>>>>>>> +{
>>>>>>> + struct platform_device *pdev = to_platform_device(dev);
>>>>>>> + struct spi_master *master = platform_get_drvdata(pdev);
>>>>>>> +
>>>>>>> + return spi_master_suspend(master);
>>>>>>
>>>>>> Won't the SPI NOR lose state across suspend ? Is that a problem ?
>>>>>
>>>>> I don't think so.
>>>>> Because when the device is not in operation and CS# is high,
>>>>> it is put in stand-by mode.
>>>>
>>>> Is the power to the SPI NOR retained ?
>>>
>>> Not if PSCI system suspend turns of power to the SoC.
>>
>> And is that a problem ?
>
> Good question!
That's what we need an answer to :)
--
Best regards,
Marek Vasut
On 12/04/2018 09:19 PM, Marek Vasut wrote:
>> Document the bindings used by the Renesas R-Car Gen3 RPC SPI controller.
>
> RPC is SPI and HF controller, it is not a pure SPI controller.
>
> How does this deal with the HF part ? Keep in mind the bindings are ABI
> and it will be difficult to redo them later.
Perhaps we need a "mode" prop, maybe w/vendor prefix?
>> Signed-off-by: Mason Yang <[email protected]>
[...]
MBR, Sergei
On 12/05/2018 07:56 PM, Sergei Shtylyov wrote:
> On 12/04/2018 09:19 PM, Marek Vasut wrote:
>
>>> Document the bindings used by the Renesas R-Car Gen3 RPC SPI controller.
>>
>> RPC is SPI and HF controller, it is not a pure SPI controller.
>>
>> How does this deal with the HF part ? Keep in mind the bindings are ABI
>> and it will be difficult to redo them later.
>
> Perhaps we need a "mode" prop, maybe w/vendor prefix?
Or, like I said last time, discern it based on the DT subnode. It can be
either SPI NOR or CFI NOR.
>>> Signed-off-by: Mason Yang <[email protected]>
> [...]
>
> MBR, Sergei
>
--
Best regards,
Marek Vasut
On 12/06/2018 06:56 AM, [email protected] wrote:
> Hi Geert,
>
>> "Geert Uytterhoeven" <[email protected]>
>> 2018/12/05 下午 05:06
>>
>> To
>>
>> [email protected],
>>
>> cc
>>
>> "Mark Brown" <[email protected]>, "Marek Vasut"
>> <[email protected]>, "Linux Kernel Mailing List" <linux-
>> [email protected]>, "linux-spi" <[email protected]>,
>> "Boris Brezillon" <[email protected]>, "Linux-Renesas"
>> <[email protected]>, "Geert Uytterhoeven" <geert
>> [email protected]>, [email protected], "Simon Horman"
>> <[email protected]>, [email protected]
>>
>> Subject
>>
>> Re: [PATCH v2 1/2] spi: Add Renesas R-Car Gen3 RPC SPI controller driver
>>
>> Hi Mason,
>>
>> On Mon, Dec 3, 2018 at 10:19 AM Mason Yang <[email protected]>
> wrote:
>> > Add a driver for Renesas R-Car Gen3 RPC SPI controller.
>> >
>> > Signed-off-by: Mason Yang <[email protected]>
>>
>> Thanks for your patch!
>>
>> > --- a/drivers/spi/Kconfig
>> > +++ b/drivers/spi/Kconfig
>> > @@ -528,6 +528,12 @@ config SPI_RSPI
>> > help
>> > SPI driver for Renesas RSPI and QSPI blocks.
>> >
>> > +config SPI_RENESAS_RPC
>> > + tristate "Renesas R-Car Gen3 RPC SPI controller"
>> > + depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
>>
>> So this driver is intended for SuperH SoCs, too?
>> If not, please drop the dependency.
>>
>
> okay, I will drop "SUPERH".
>
>> > --- /dev/null
>> > +++ b/drivers/spi/spi-renesas-rpc.c
>>
>> > +#ifdef CONFIG_RESET_CONTROLLER
>> > +static int rpc_spi_do_reset(struct rpc_spi *rpc)
>>
>> What's the purpose of the reset routine?
>
> in case RPC xfer is time-out due to something wrong in RPC module,
> as Marek comments.
>
>> Given the #ifdef, is it optional or required?
>>
>> > +{
>> > + int i, ret;
>> > +
>> > + ret = reset_control_reset(rpc->rstc);
>> > + if (ret)
>> > + return ret;
>> > +
>> > + for (i = 0; i < LOOP_TIMEOUT; i++) {
>> > + ret = reset_control_status(rpc->rstc);
>> > + if (ret == 0)
>> > + return 0;
>> > + usleep_range(0, 1);
>> > + }
>>
>> Why do you need this loop?
>> The delay in cpg_mssr_reset() should be sufficient.
>>
>
> yup, I know there is already 35 us delay in cpg_mssr_reset().
> If you think reset_control_status()checking is not necessary,
> I will drop it.
>
>> > +
>> > + return -ETIMEDOUT;
>> > +}
>> > +#else
>> > +static int rpc_spi_do_reset(struct rpc_spi *rpc)
>> > +{
>> > + return -ETIMEDOUT;
>> > +}
>> > +#endif
>>
>> > +static int rpc_spi_transfer_one_message(struct spi_master *master,
>> > + struct spi_message *msg)
>> > +{
>> > + struct rpc_spi *rpc = spi_master_get_devdata(master);
>> > + struct spi_transfer *t;
>> > + int ret;
>> > +
>> > + rpc_spi_transfer_setup(rpc, msg);
>> > +
>> > + list_for_each_entry(t, &msg->transfers, transfer_list) {
>> > + if (!list_is_last(&t->transfer_list, &msg->transfers))
>> > + continue;
>> > + ret = rpc_spi_xfer_message(rpc, t);
>>
>> rpc_spi_xfer_message() sounds like a bad name to me, given it operates
>> on a transfer, not on a message.
>>
>
> Because RPC send a entire SPI message at one time, not separately,
> that is the 1'st transfer is for command, the 2'nd transfer is for
> address/data
> and so on.
> The reason is CS# pin control restriction in RPC HW module.
>
>
>> > + if (ret)
>> > + goto out;
>> > + }
>> > +
>> > + msg->status = 0;
>> > + msg->actual_length = rpc->totalxferlen;
>> > +out:
>> > + spi_finalize_current_message(master);
>> > + return 0;
>> > +}
>>
>>
>> > +static int rpc_spi_probe(struct platform_device *pdev)
>> > +{
>>
>> > + rpc->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
>> > + if (IS_ERR(rpc->rstc))
>> > + return PTR_ERR(rpc->rstc);
>>
>> This will return an error if CONFIG_RESET_CONTROLLER is not set, hence
>> the #ifdef above is moot.
>>
>
> You are right.
> so, I should do
> Option 1: remove #CONFIG_RESET_CONTROLLER
> Option 2: add #CONFIG_RESET_CONTROLLER for
> devm_reset_control_get_exclusive()
>
> please comments on it, thanks.
>
>
>> > +
>> > + 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;
>>
>> Is there any reason you cannot use the standard
>> spi_transfer_one_message, i.e. provide spi_controller.transfer_one()
>> instead of spi_controller.transfer_one_message()?
>>
>
> It seems there is a RPC HW restriction in CS# pin control.
> Therefore, it can't send the 1'st spi-transfer for command and then
> keeping CS# pin goes low for the 2'nd spi-transfer for address/data and
> so on.
Isn't register DRCR bit SSLN/SSLE exactly for this purpose ?
--
Best regards,
Marek Vasut
On 12/06/2018 08:30 AM, [email protected] wrote:
> Hi Marek,
Hi,
>> >> Re: [PATCH v2 1/2] spi: Add Renesas R-Car Gen3 RPC SPI controller
> driver
>> >>
>> >> On 12/03/2018 10:18 AM, Mason Yang wrote:
>> >> > Add a driver for Renesas R-Car Gen3 RPC SPI controller.
>> >> >
>> >> > Signed-off-by: Mason Yang <[email protected]>
>> >>
>> >> What changed in this V2 ?
>> >>
>> >> [...]
>> >
>> > see some description in [PATH v2 0/2]
>>
>> I don't see any V2: list there.
>>
>
> including
> 1) remove RPC clock enable/dis-able control,
> 2) patch run time PM,
> 3) add RPC module software reset,
> 4) add regmap,
> 5) other coding style and so on.
Please include a detailed changelog in each subsequent patch series.
>> >> > +static int rpc_spi_io_xfer(struct rpc_spi *rpc,
>> >> > + ? ? ? ? ? ?const void *tx_buf, void *rx_buf)
>> >> > +{
>> >> > + ? u32 smenr, smcr, data, pos = 0;
>> >> > + ? int ret = 0;
>> >> > +
>> >> > + ? regmap_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD |
> RPC_CMNCR_SFDE |
>> >> > + ? ? ? ? ? ? ?RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
>> >> > + ? ? ? ? ? ? ?RPC_CMNCR_BSZ(0));
>> >> > + ? regmap_write(rpc->regmap, RPC_SMDRENR, 0x0);
>> >> > + ? regmap_write(rpc->regmap, RPC_SMCMR, rpc->cmd);
>> >> > + ? regmap_write(rpc->regmap, RPC_SMDMCR, rpc->dummy);
>> >> > + ? regmap_write(rpc->regmap, RPC_SMADR, rpc->addr);
>> >> > +
>> >> > + ? if (tx_buf) {
>> >> > + ? ? ?smenr = rpc->smenr;
>> >> > +
>> >> > + ? ? ?while (pos < rpc->xferlen) {
>> >> > + ? ? ? ? u32 nbytes = rpc->xferlen ?- pos;
>> >> > +
>> >> > + ? ? ? ? regmap_write(rpc->regmap, RPC_SMWDR0,
>> >> > + ? ? ? ? ? ? ? ? *(u32 *)(tx_buf + pos));
>> >>
>> >> *(u32 *) cast is probably not needed , fix casts globally.
>> >
>> > It must have it!
>>
>> Why ?
>
> Get a compiler warning due to tx_bug is void *, as Geert replied.
The compiler warning is usually an indication that this is something to
check, not silence with a type cast.
> Using get_unaligned(), patched code would be
> -----------------------------------------------------
> regmap_write(rpc->regmap, RPC_SMWDR0,
> ? ? ? ? ? ? ? ? ?get_unaligned((u32 *)(tx_buf + pos))); ? ? ? ? ? ? ? ?
> ----------------------------------------------------
Do you need the cast if you use get_unaligned() ?
>> >> > + ? ? ? ? rpc->xferlen = *(u32 *)len;
>> >> > + ? ? ? ? rpc->totalxferlen += *(u32 *)len;
>> >> > + ? ? ?} else {
>> >> > + ? ? ? ? rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer
>> >> > + ? ? ? ? ? ? ? (op->data.nbytes)) | RPC_SMENR_SPIDB
>> >> > + ? ? ? ? ? ? ? (fls(op->data.buswidth >> 1));
>> >>
>> >> Drop parenthesis around fls()
>> >
>> > ?
>> > no way.
>>
>> I would really appreciate it if you could explain things instead.
>>
>> Geert already did so, by pointing out this is a confusing code
>> formatting problem and how it should be fixed, so no need to repeat
>> that. But I hope you understand how that sort of explanation is far more
>> valuable than "no way" kind of reply.
>
> okay, understood.
>
>
>> >> > +
>> >> > + ? xfercnt = xferpos;
>> >> > + ? rpc->xferlen = xfer[--xferpos].len;
>> >> > + ? rpc->cmd = RPC_SMCMR_CMD(((u8 *)xfer[0].tx_buf)[0]);
>> >>
>> >> Is the cast needed ?
>> >
>> > yes!
>>
>> Why ?
>
> Get a compiler warning due to tx_bug is void *, as Geert replied.
> Using get_unaligned(), patched code would be
> ---------------------------------------------------------------
> ?rpc->cmd = RPC_SMCMR_CMD(get_unaligned((u8 *)xfer[0].tx_buf));
> ----------------------------------------------------------------
See above
>> >> > + ? 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));
>> >> > + ? ? ?} 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));
>> >> > + ? ? ?}
>> >> > + ? ? ?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));
>> >>
>> >> It seems this SMENR pattern repeats itself throughout the driver, can
>> >> this be improved / deduplicated ?
>> >
>> > It seems no way!
>>
>> Why ?
>
> okay, I patch this with for( ) loop.
Please do, let's see how it looks .
>> >> > + ? ? ?} 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));
>> >> > + ? ? ?}
>> >> > + ? ? ?break;
>> >> > +
>> >> > + ? case 4:
>> >> > + ? ? ?if (xfer[2].len && xfer[2].tx_buf) {
>> >> > + ? ? ? ? rpc->smenr |= RPC_SMENR_DME;
>> >> > + ? ? ? ? rpc->dummy = RPC_SMDMCR_DMCYC(xfer[2].len);
>> >> > + ? ? ?}
>> >> > +
>> >> > + ? ? ?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));
>> >> > + ? ? ?}
>> >> > + ? ? ?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))
>> >> > + ? ? ? ? continue;
>> >> > + ? ? ?ret = rpc_spi_xfer_message(rpc, t);
>> >> > + ? ? ?if (ret)
>> >> > + ? ? ? ? goto out;
>> >> > + ? }
>> >> > +
>> >> > + ? msg->status = 0;
>> >> > + ? msg->actual_length = rpc->totalxferlen;
>> >> > +out:
>> >> > + ? spi_finalize_current_message(master);
>> >> > + ? return 0;
>> >> > +}
>> >> > +
>> >> > +static const struct regmap_range rpc_spi_volatile_ranges[] = {
>> >> > + ? regmap_reg_range(RPC_SMRDR0, RPC_SMRDR0),
>> >> > + ? regmap_reg_range(RPC_SMWDR0, RPC_SMWDR0),
>> >>
>> >> Why is SMWDR volatile ?
>> >
>> > No matter is write-back or write-through.
>>
>> Oh, do you want to assure the SMWDR value is always written directly to
>> the hardware ?
>
> yes,
>
>>
>> btw. I think SMRDR should be precious ?
>
> so, you think I should drop
> regmap_reg_range(RPC_SMWDR0, RPC_SMWDR0)
No, I am asking whether SMRDR should be marked precious or not.
[...]
> 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.
Can you do something about this ^ please ?
--
Best regards,
Marek Vasut
Hi Mason,
On Thu, Dec 6, 2018 at 8:31 AM <[email protected]> wrote:
> > >> Re: [PATCH v2 1/2] spi: Add Renesas R-Car Gen3 RPC SPI controller driver
> > >>
> > >> On 12/03/2018 10:18 AM, Mason Yang wrote:
> > >> > Add a driver for Renesas R-Car Gen3 RPC SPI controller.
> > >> >
> > >> > Signed-off-by: Mason Yang <[email protected]>
> > >> > + xfercnt = xferpos;
> > >> > + rpc->xferlen = xfer[--xferpos].len;
> > >> > + rpc->cmd = RPC_SMCMR_CMD(((u8 *)xfer[0].tx_buf)[0]);
> > >>
> > >> Is the cast needed ?
> > >
> > > yes!
> >
> > Why ?
>
> Get a compiler warning due to tx_bug is void *, as Geert replied.
>
> Using get_unaligned(), patched code would be
> ---------------------------------------------------------------
> rpc->cmd = RPC_SMCMR_CMD(get_unaligned((u8 *)xfer[0].tx_buf));
> ----------------------------------------------------------------
Using get_unaligned(0 is a bit strange for accessing a single byte quantity.
Please keep the normal pointer dereference (including the cast).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 12/06/2018 10:12 AM, Geert Uytterhoeven wrote:
> Hi Mason,
>
> On Thu, Dec 6, 2018 at 8:31 AM <[email protected]> wrote:
>>>>> Re: [PATCH v2 1/2] spi: Add Renesas R-Car Gen3 RPC SPI controller driver
>>>>>
>>>>> On 12/03/2018 10:18 AM, Mason Yang wrote:
>>>>>> Add a driver for Renesas R-Car Gen3 RPC SPI controller.
>>>>>>
>>>>>> Signed-off-by: Mason Yang <[email protected]>
>
>>>>>> + xfercnt = xferpos;
>>>>>> + rpc->xferlen = xfer[--xferpos].len;
>>>>>> + rpc->cmd = RPC_SMCMR_CMD(((u8 *)xfer[0].tx_buf)[0]);
>>>>>
>>>>> Is the cast needed ?
>>>>
>>>> yes!
>>>
>>> Why ?
>>
>> Get a compiler warning due to tx_bug is void *, as Geert replied.
>>
>> Using get_unaligned(), patched code would be
>> ---------------------------------------------------------------
>> rpc->cmd = RPC_SMCMR_CMD(get_unaligned((u8 *)xfer[0].tx_buf));
>> ----------------------------------------------------------------
>
> Using get_unaligned(0 is a bit strange for accessing a single byte quantity.
> Please keep the normal pointer dereference (including the cast).
Oh, right, for single bytes this is OK.
--
Best regards,
Marek Vasut
On 12/06/2018 10:17 AM, [email protected] wrote:
> Hi Marek,
Hi,
>> >> > +
>> >> > + ? ? ? 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;
>> >>
>> >> Is there any reason you cannot use the standard
>> >> spi_transfer_one_message, i.e. provide spi_controller.transfer_one()
>> >> instead of spi_controller.transfer_one_message()?
>> >>
>> >
>> > It seems there is a RPC HW restriction in CS# pin control.
>> > Therefore, it can't send the 1'st spi-transfer for command and then
>> > keeping CS# pin goes low for the 2'nd spi-transfer for address/data and
>> > so on.
>>
>> Isn't register DRCR bit SSLN/SSLE exactly for this purpose ?
>>
>
> DRCR is for RPC module works in external space read mode, using memcpy( ).
> It is not for _spi_sync().
>
> I only could use manual I/O mode by SMCR@bit-8 SSLKP and I found it has
> some
> restrictions in manual I/O mode to control CS# pin.
What restrictions are those ? I am aware of some, maybe there is more.
--
Best regards,
Marek Vasut
On 12/07/2018 08:24 AM, [email protected] wrote:
>
> Hi Marek,
Hi,
>> >> >> > +
>> >> >> > + ? ? ? ? regmap_write(rpc->regmap, RPC_SMWDR0,
>> >> >> > + ? ? ? ? ? ? ? ? *(u32 *)(tx_buf + pos));
>> >> >>
>> >> >> *(u32 *) cast is probably not needed , fix casts globally.
>> >> >
>> >> > It must have it!
>> >>
>> >> Why ?
>> >
>> > Get a compiler warning due to tx_bug is void *, as Geert replied.
>>
>> The compiler warning is usually an indication that this is something to
>> check, not silence with a type cast.
>>
>> > Using get_unaligned(), patched code would be
>> > -----------------------------------------------------
>> > regmap_write(rpc->regmap, RPC_SMWDR0,
>> > ? ? ? ? ? ? ? ? ?get_unaligned((u32 *)(tx_buf + pos))); ? ? ? ? ? ? ? ?
>> > ----------------------------------------------------
>>
>> Do you need the cast if you use get_unaligned() ?
>
> yes!
Why ? (you can drop one iteration here by explaining why right away,
since I'll ask for that explanation for sure)
>> >> >> > +
>> >> >> > +static const struct regmap_range rpc_spi_volatile_ranges[] = {
>> >> >> > + ? regmap_reg_range(RPC_SMRDR0, RPC_SMRDR0),
>> >> >> > + ? regmap_reg_range(RPC_SMWDR0, RPC_SMWDR0),
>> >> >>
>> >> >> Why is SMWDR volatile ?
>> >> >
>> >> > No matter is write-back or write-through.
>> >>
>> >> Oh, do you want to assure the SMWDR value is always written directly to
>> >> the hardware ?
>> >
>> > yes,
>> >
>> >>
>> >> btw. I think SMRDR should be precious ?
>> >
>> > so, you think I should drop
>> > regmap_reg_range(RPC_SMWDR0, RPC_SMWDR0)
>>
>> No, I am asking whether SMRDR should be marked precious or not.
>>
>
> I don't think so,
> precious_reg: the register should not be read outside of
> call from driver, i.e,. a clear on read interrupt status register.
If you read the reg outside of the call from driver, it will mess up the
internal FIFO and the whole driver will get into undefined state, right?
Maybe Mark can chime in ?
>> [...]
>>
>> > 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.
>>
>> Can you do something about this ^ please ?
>>
>
> I submit the patch file is by another remote git-server, which
> supports git-format patch, git send-mail and so on.
> But it can't receive email.
>
> I get/send email are by office PC w/ Notes system and
> this "CONFx NOTE:" is appended automatically by Notes mail-server.
>
> Please just never mind it.
I am concerned MXIC can sue everyone on the CC list of this email based
solely on your suggestion to ignore this paragraph. That's not a good
position to be in.
--
Best regards,
Marek Vasut
Hello!
I'd already started the v2 driver review before you posted v3, so here goes...
On 12/03/2018 12:18 PM, Mason Yang wrote:
> Add a driver for Renesas R-Car Gen3 RPC SPI controller.
>
> Signed-off-by: Mason Yang <[email protected]>
[...]
> diff --git a/drivers/spi/spi-renesas-rpc.c b/drivers/spi/spi-renesas-rpc.c
> new file mode 100644
> index 0000000..ac9094e
> --- /dev/null
> +++ b/drivers/spi/spi-renesas-rpc.c
> @@ -0,0 +1,808 @@
[...]
> +#define RPC_CMNCR 0x0000 /* R/W */
> +#define RPC_CMNCR_MD BIT(31)
> +#define RPC_CMNCR_SFDE BIT(24)
This bit is undocumented as of the gen3 manual v1.0. I'd like this to be reflected
in a comment...
[...]
> +#define RPC_CMNCR_IO3FV(val) (((val) & 0x3) << 14)
> +#define RPC_CMNCR_IO2FV(val) (((val) & 0x3) << 12)
These 2 are also undocumented.
[...]
> +#define RPC_CMNCR_CPHAT BIT(6)
> +#define RPC_CMNCR_CPHAR BIT(5)
> +#define RPC_CMNCR_SSLP BIT(4)
> +#define RPC_CMNCR_CPOL BIT(3)
And these 4...
> +#define RPC_DRCR 0x000C /* R/W */
> +#define RPC_DRCR_SSLN BIT(24)
> +#define RPC_DRCR_RBURST(v) (((v) & 0x1F) << 16)
More like ((v) - 1), like in another register...
> +#define RPC_DRCR_RCF BIT(9)
> +#define RPC_DRCR_RBE BIT(8)
> +#define RPC_DRCR_SSLE BIT(0)
[...]
> +#define RPC_DREAR 0x0014 /* R/W */
> +#define RPC_DREAR_EAC BIT(0)
The manual says it takes bits 0 to 2...
[...]
> +#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)
You either go in descending or ascending order, not both. :-)
[...]
> +#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)
The manual calls this field PHYMEM...
[...]
> +#define LOOP_TIMEOUT 1024
It's more like TIMEOUT_LOOPS. :-)
[...]
> +static void rpc_spi_hw_init(struct rpc_spi *rpc)
> +{
> + /*
> + * NOTE: The 0x260 are undocumented bits, but they must be set.
> + * RPC_PHYCNT_STRTIM is strobe timing adjustment bit,
> + * 0x0 : the delay is biggest,
> + * 0x1 : the delay is 2nd biggest,
> + * 0x3 or 0x6 is a recommended value.
> + */
> + regmap_write(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_CAL |
> + RPC_PHYCNT_STRTIM(0x6) | 0x260);
> +
> + /*
> + * NOTE: The 0x31511144 and 0x431 are undocumented bits,
> + * but they must be set for RPC_PHYOFFSET1 & RPC_PHYOFFSET2.
> + */
> + regmap_write(rpc->regmap, RPC_PHYOFFSET1, 0x31511144);
> + regmap_write(rpc->regmap, RPC_PHYOFFSET2, 0x431);
0x400 is actually documented, bits 0..7 are read only and default to 0x31...
> +#ifdef CONFIG_RESET_CONTROLLER
> +static int rpc_spi_do_reset(struct rpc_spi *rpc)
> +{
> + int i, ret;
> +
> + ret = reset_control_reset(rpc->rstc);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < LOOP_TIMEOUT; i++) {
> + ret = reset_control_status(rpc->rstc);
> + if (ret == 0)
> + return 0;
> + usleep_range(0, 1);
Are you serious? :-)
> + }
> +
> + return -ETIMEDOUT;
> +}
> +#else
> +static int rpc_spi_do_reset(struct rpc_spi *rpc)
> +{
> + return -ETIMEDOUT;
> +}
> +#endif
[...]
> +static int rpc_spi_io_xfer(struct rpc_spi *rpc,
> + const void *tx_buf, void *rx_buf)
> +{
> + u32 smenr, smcr, data, pos = 0;
> + int ret = 0;
> +
> + regmap_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD | RPC_CMNCR_SFDE |
> + RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
> + RPC_CMNCR_BSZ(0));
> + regmap_write(rpc->regmap, RPC_SMDRENR, 0x0);
Just 0, please.
> + regmap_write(rpc->regmap, RPC_SMCMR, rpc->cmd);
> + regmap_write(rpc->regmap, RPC_SMDMCR, rpc->dummy);
> + regmap_write(rpc->regmap, RPC_SMADR, rpc->addr);
> +
> + if (tx_buf) {
> + smenr = rpc->smenr;
> +
> + while (pos < rpc->xferlen) {
> + u32 nbytes = rpc->xferlen - pos;
One space before '-' is enough. :-)
> +
> + regmap_write(rpc->regmap, RPC_SMWDR0,
> + *(u32 *)(tx_buf + pos));
Ugh... what about the data endianness?
> +
> + if (nbytes > 4) {
> + nbytes = 4;
> + smcr = rpc->smcr |
> + RPC_SMCR_SPIE | RPC_SMCR_SSLKP;
> + } else {
> + smcr = rpc->smcr | RPC_SMCR_SPIE;
> + }
Please do this repeated part outside *if*:
smcr = rpc->smcr | RPC_SMCR_SPIE;
if (nbytes > 4) {
nbytes = 4;
smcr |= RPC_SMCR_SSLKP;
}
[...]
> + } else if (rx_buf) {
> + while (pos < rpc->xferlen) {
> + u32 nbytes = rpc->xferlen - pos;
Again, 1 space is enough...
> +
> + if (nbytes > 4)
> + nbytes = 4;
> +
> + regmap_write(rpc->regmap, RPC_SMENR, rpc->smenr);
> + regmap_write(rpc->regmap, RPC_SMCR,
> + rpc->smcr | RPC_SMCR_SPIE);
> + ret = wait_msg_xfer_end(rpc);
> + if (ret)
> + goto out;
> +
> + regmap_read(rpc->regmap, RPC_SMRDR0, &data);
> + memcpy_fromio(rx_buf + pos, (void *)&data, nbytes);
What?! The 'data' variable is not in a MMIO region, you can use plain memcpy().
Not sure about the endianness tho...
[...]
> + } else {
> + regmap_write(rpc->regmap, RPC_SMENR, rpc->smenr);
> + regmap_write(rpc->regmap, RPC_SMCR, rpc->smcr | RPC_SMCR_SPIE);
Er... what's this for?
> + ret = wait_msg_xfer_end(rpc);
> + if (ret)
> + goto out;
> + }
> +
> + return ret;
Need empty line here...
> +out:
> + return rpc_spi_do_reset(rpc);
> +}
> +
> +static void rpc_spi_mem_set_prep_op_cfg(struct spi_device *spi,
> + const struct spi_mem_op *op,
> + u64 *offs, size_t *len)
> +{
> + struct rpc_spi *rpc = spi_master_get_devdata(spi->master);
> +
> + rpc->cmd = RPC_SMCMR_CMD(op->cmd.opcode);
> + rpc->smenr = RPC_SMENR_CDE |
> + RPC_SMENR_CDB(fls(op->cmd.buswidth >> 1));
Maybe ilog2()?
> + rpc->totalxferlen = 1;
> + rpc->xfer_dir = SPI_MEM_NO_DATA;
> + rpc->xferlen = 0;
> + rpc->addr = 0;
> +
> + if (op->addr.nbytes) {
> + rpc->smenr |= RPC_SMENR_ADB(fls(op->addr.buswidth >> 1));
Again, ilog2()?
[...]
> + if (op->data.nbytes || (offs && len)) {
> + if (op->data.dir == SPI_MEM_DATA_IN) {
> + rpc->smcr = RPC_SMCR_SPIRE;
> + rpc->xfer_dir = SPI_MEM_DATA_IN;
> + } else if (op->data.dir == SPI_MEM_DATA_OUT) {
> + rpc->smcr = RPC_SMCR_SPIWE;
> + rpc->xfer_dir = SPI_MEM_DATA_OUT;
> + }
This code is asking for using *switch*...
> +
> + if (offs && len) {
> + rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer
> + (*(u32 *)len)) | RPC_SMENR_SPIDB
Unobvious line breaks...
> + (fls(op->data.buswidth >> 1));
ilog2()?
> +
> + rpc->xferlen = *(u32 *)len;
> + rpc->totalxferlen += *(u32 *)len;
> + } else {
> + rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer
> + (op->data.nbytes)) | RPC_SMENR_SPIDB
> + (fls(op->data.buswidth >> 1));
You can factor out a large part of this expression and calculate it before *if*...
> +
> + 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)
Hm, the manual doesn't mention 2-wire SPI mode...
> + 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);
> +
> + regmap_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_SFDE |
> + RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
> + RPC_CMNCR_BSZ(0));
> + regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RBURST(0x1f) |
RPC_DRCR_RBURST(32), please.
> + RPC_DRCR_RBE);
> + regmap_write(rpc->regmap, RPC_DRCMR, rpc->cmd);
> + regmap_write(rpc->regmap, RPC_DREAR, RPC_DREAR_EAC);
> + regmap_write(rpc->regmap, RPC_DROPR, 0);
> + regmap_write(rpc->regmap, RPC_DRENR, rpc->smenr);
Looks liky you need a more generic field name (like rpc->cmd)...
> + regmap_write(rpc->regmap, RPC_DRDMCR, rpc->dummy);
> + regmap_write(rpc->regmap, RPC_DRDRENR, 0);
> +
> + memcpy_fromio(buf, rpc->linear.map + desc->info.offset + offs, len);
What if the read direct-mapped area is less than U32_MAX in size (and it will be,
according to your bindings)?
> +
> + return len;
> +}
> +
> +static ssize_t rpc_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
> + u64 offs, size_t len, const void *buf)
> +{
> + struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
> + int ret;
> +
> + if (WARN_ON(offs + desc->info.offset + len > U32_MAX))
> + return -EINVAL;
> +
> + if (WARN_ON(len > RPC_WBUF_SIZE))
> + return -EIO;
Why not write 256 bytes and return w/that?
> +
> + ret = rpc_spi_set_freq(rpc, desc->mem->spi->max_speed_hz);
> + if (ret)
> + return ret;
> +
> + rpc_spi_mem_set_prep_op_cfg(desc->mem->spi,
> + &desc->info.op_tmpl, &offs, &len);
> +
> + regmap_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD | RPC_CMNCR_SFDE |
> + RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
> + RPC_CMNCR_BSZ(0));
> + regmap_write(rpc->regmap, RPC_SMDRENR, 0);
> + regmap_write(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_CAL | 0x260 |
> + RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF);
Didn't understand this 2-write-buffers thing...
> +
> + memcpy_toio(rpc->base + RPC_WBUF, buf, len);
> +
> + regmap_write(rpc->regmap, RPC_SMCMR, rpc->cmd);
> + regmap_write(rpc->regmap, RPC_SMADR, offs);
> + regmap_write(rpc->regmap, RPC_SMENR, rpc->smenr);
> + regmap_write(rpc->regmap, RPC_SMCR, rpc->smcr | RPC_SMCR_SPIE);
> + ret = wait_msg_xfer_end(rpc);
> + if (ret)
> + goto out;
> +
> + regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RCF);
> + regmap_write(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_CAL |
> + RPC_PHYCNT_STRTIM(6) | 0x260);
> +
> + return len;
Need empty line here.
> +out:
> + return rpc_spi_do_reset(rpc);
> +}
[...]> +static void rpc_spi_transfer_setup(struct rpc_spi *rpc,
> + struct spi_message *msg)
> +{
> + struct spi_transfer *t, xfer[4] = { };
> + u32 i, xfercnt, xferpos = 0;
> +
> + rpc->totalxferlen = 0;
> + rpc->xfer_dir = SPI_MEM_NO_DATA;
> +
> + list_for_each_entry(t, &msg->transfers, transfer_list) {
> + if (t->tx_buf) {
> + xfer[xferpos].tx_buf = t->tx_buf;
> + xfer[xferpos].tx_nbits = t->tx_nbits;
> + }
> +
> + if (t->rx_buf) {
> + xfer[xferpos].rx_buf = t->rx_buf;
> + xfer[xferpos].rx_nbits = t->rx_nbits;
> + }
> +
> + if (t->len) {
> + xfer[xferpos++].len = t->len;
> + rpc->totalxferlen += t->len;
> + }
> +
> + if (list_is_last(&t->transfer_list, &msg->transfers)) {
> + if (xferpos > 1 && t->rx_buf) {
> + rpc->xfer_dir = SPI_MEM_DATA_IN;
> + rpc->smcr = RPC_SMCR_SPIRE;
> + } else if (xferpos > 1 && t->tx_buf) {
Why check 'xferpos' twice?
> + rpc->xfer_dir = SPI_MEM_DATA_OUT;
> + rpc->smcr = RPC_SMCR_SPIWE;
> + }
> + }
> + }
> +
> + xfercnt = xferpos;
> + rpc->xferlen = xfer[--xferpos].len;
> + rpc->cmd = RPC_SMCMR_CMD(((u8 *)xfer[0].tx_buf)[0]);
> + rpc->smenr = RPC_SMENR_CDE | RPC_SMENR_CDB(fls(xfer[0].tx_nbits >> 1));
ilog2()?
> + rpc->addr = 0;
> +
> + if (xfercnt > 2 && xfer[1].len && xfer[1].tx_buf) {
> + rpc->smenr |= RPC_SMENR_ADB(fls(xfer[1].tx_nbits >> 1));
ilog2()?
> + for (i = 0; i < xfer[1].len; i++)
> + rpc->addr |= (u32)((u8 *)xfer[1].tx_buf)[i]
> + << (8 * (xfer[1].len - i - 1));
Ugh, you need get_unaligned_*()...
> +
> + 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));
> + } 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));
> + }
> + 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));
> + } 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));
> + }
> + break;
> +
> + case 4:
> + if (xfer[2].len && xfer[2].tx_buf) {
> + rpc->smenr |= RPC_SMENR_DME;
> + rpc->dummy = RPC_SMDMCR_DMCYC(xfer[2].len);
> + }
> +
> + 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));
> + }
> + break;
> +
> + default:
> + break;
Ugh... don't want to repeat after Marek. :-)
[...]
> +static const struct regmap_config rpc_spi_regmap_config = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 4,
> + .fast_io = true,
> + .max_register = RPC_WBUF + RPC_WBUF_SIZE,
Ugh... 0x8100/4 regs, of which the majority isn't used... :-/
> + .volatile_table = &rpc_spi_volatile_table,
> +};
> +
> +static int rpc_spi_probe(struct platform_device *pdev)
> +{
> + struct spi_master *master;
> + struct resource *res;
> + struct rpc_spi *rpc;
> + const struct regmap_config *regmap_config;
> + int ret;
[...]
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "rpc_regs");
I'd suggest just "regs".
> + rpc->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(rpc->base))
> + return PTR_ERR(rpc->base);
[...]
> +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);
No spi_master_put() here?
[...]
MBR, Sergei
On 12/07/2018 07:17 PM, Sergei Shtylyov wrote:
> Hello!
>
> I'd already started the v2 driver review before you posted v3, so here goes...
>
> On 12/03/2018 12:18 PM, Mason Yang wrote:
>
>> Add a driver for Renesas R-Car Gen3 RPC SPI controller.
>>
>> Signed-off-by: Mason Yang <[email protected]>
> [...]
>> diff --git a/drivers/spi/spi-renesas-rpc.c b/drivers/spi/spi-renesas-rpc.c
>> new file mode 100644
>> index 0000000..ac9094e
>> --- /dev/null
>> +++ b/drivers/spi/spi-renesas-rpc.c
>> @@ -0,0 +1,808 @@
> [...]
>> +#define RPC_CMNCR 0x0000 /* R/W */
>> +#define RPC_CMNCR_MD BIT(31)
>> +#define RPC_CMNCR_SFDE BIT(24)
>
> This bit is undocumented as of the gen3 manual v1.0. I'd like this to be reflected
> in a comment...
FYI, not even in v1.50 .
--
Best regards,
Marek Vasut
Hello!
On 12/11/2018 12:26 PM, [email protected] wrote:
[...]
>> I'd already started the v2 driver review before you posted v3, so
>> here goes...
>>
>> On 12/03/2018 12:18 PM, Mason Yang wrote:
>>
>>> Add a driver for Renesas R-Car Gen3 RPC SPI controller.
>>>
>>> Signed-off-by: Mason Yang <[email protected]>
>> [...]
>>> diff --git a/drivers/spi/spi-renesas-rpc.c b/drivers/spi/spi-renesas-rpc.c
>>> new file mode 100644
>>> index 0000000..ac9094e
>>> --- /dev/null
>>> +++ b/drivers/spi/spi-renesas-rpc.c
>>> @@ -0,0 +1,808 @@
>> [...]
>>> +#define RPC_DRCR 0x000C /* R/W */
>>> +#define RPC_DRCR_SSLN BIT(24)
>>> +#define RPC_DRCR_RBURST(v) (((v) & 0x1F) << 16)
>>
>> More like ((v) - 1), like in another register...
I mean you can pass the read data burst length as a # of data units,
thus just substracting 1, like you did in the other case...
>> [...]
>>> +#define RPC_DREAR 0x0014 /* R/W */
>>> +#define RPC_DREAR_EAC BIT(0)
>>
>> The manual says it takes bits 0 to 2...
>
> yup, EAC[2:0]
> but as datasheet description, either 0 or 1 is OK to BIT(0),
> other than above setting is prohibited
I'd prefer that this matches the manual. #define the values or
just pass them to RPC_DREAR_EAC(v).
>> [...]
>>> +#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)
>>
>> You either go in descending or ascending order, not both. :-)
>
> I can't get your point.
You #define in the ascending order of the bit # (shift count) here and
in the descending order elsewhere.
[...]
>>> +static void rpc_spi_hw_init(struct rpc_spi *rpc)
>>> +{
>>> + /*
>>> + * NOTE: The 0x260 are undocumented bits, but they must be set.
>>> + * RPC_PHYCNT_STRTIM is strobe timing adjustment bit,
>>> + * 0x0 : the delay is biggest,
>>> + * 0x1 : the delay is 2nd biggest,
>>> + * 0x3 or 0x6 is a recommended value.
>>> + */
>>> + regmap_write(rpc->regmap, RPC_PHYCNT, RPC_PHYCNT_CAL |
>>> + RPC_PHYCNT_STRTIM(0x6) | 0x260);
>>> +
>>> + /*
>>> + * NOTE: The 0x31511144 and 0x431 are undocumented bits,
>>> + * but they must be set for RPC_PHYOFFSET1 & RPC_PHYOFFSET2.
>>> + */
>>> + regmap_write(rpc->regmap, RPC_PHYOFFSET1, 0x31511144);
>>> + regmap_write(rpc->regmap, RPC_PHYOFFSET2, 0x431);
>>
>> 0x400 is actually documented, bits 0..7 are read only and defaultto 0x31...
> I got these values from R-Car bare-metal code, mini-Monitor v4.01.
>
> What should I describe these bits 0x400 and 0x31 if it is needed?
#define PHYOFFSET2_OCTTMG(v) ((v) & 0x7) << 8)
The read-modify-write operation ios preferred in this casa, so that
0x31 doesn't appear anywhere.
[...]
>>> +
>>> + if (nbytes > 4) {
>>> + nbytes = 4;
>>> + smcr = rpc->smcr |
>>> + RPC_SMCR_SPIE | RPC_SMCR_SSLKP;
>>> + } else {
>>> + smcr = rpc->smcr | RPC_SMCR_SPIE;
>>> + }
>>
>> Please do this repeated part outside *if*:
>
> ?
> The procedure is different !
Where?!
>> smcr = rpc->smcr | RPC_SMCR_SPIE;
>> if (nbytes > 4) {
>> nbytes = 4;
>> smcr |= RPC_SMCR_SSLKP;
>> }
[...]
>>> +
>>> + if (nbytes > 4)
>>> + nbytes = 4;
>>> +
>>> + regmap_write(rpc->regmap, RPC_SMENR, rpc->smenr);
>>> + regmap_write(rpc->regmap, RPC_SMCR,
>>> + rpc->smcr | RPC_SMCR_SPIE);
>>> + ret = wait_msg_xfer_end(rpc);
>>> + if (ret)
>>> + goto out;
>>> +
>>> + regmap_read(rpc->regmap, RPC_SMRDR0, &data);
>>> + memcpy_fromio(rx_buf + pos, (void *)&data, nbytes);
>>
>> What?! The 'data' variable is not in a MMIO region, you can use
>> plain memcpy().
>> Not sure about the endianness tho...
>
> yup, the 'data' variable is not in MMIO region!
> patch it to memcpy() rather than memcpy_fromio().
Also, pointer casts to 'void *' are automatic...
[...]
>>> +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);
>>> +
>>> + regmap_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_SFDE |
>>> + RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
>>> + RPC_CMNCR_BSZ(0));
>>> + regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RBURST(0x1f) |
>>
>> RPC_DRCR_RBURST(32), please.
>
> ?
> the max value is 31 = 0x1f
See above!
[...]
>>> + regmap_write(rpc->regmap, RPC_DRDMCR, rpc->dummy);
>>> + regmap_write(rpc->regmap, RPC_DRDRENR, 0);
>>> +
>>> + memcpy_fromio(buf, rpc->linear.map + desc->info.offset + offs, len);
>>
>> What if the read direct-mapped area is less than U32_MAX in size
>> (and it will be,
>> according to your bindings)?
>
> the max direct mapping read area is 64 KByte description in DTS.
You don't check for this before reading (but you do for writing)!
[...]
>>> +static ssize_t rpc_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
>>> + u64 offs, size_t len, const void *buf)
>>> +{
>>> + struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
>>> + int ret;
>>> +
>>> + if (WARN_ON(offs + desc->info.offset + len > U32_MAX))
>>> + return -EINVAL;
>>> +
>>> + if (WARN_ON(len > RPC_WBUF_SIZE))
>>> + return -EIO;
>>
>> Why not write 256 bytes and return w/that?
>
> in manual 62.3.13 Write Buffer Operation,
> transfer size to device is 256-byte unit.
Why not write 256 bytes max and just return 256?
[...]
>> [...]
>>> +static void rpc_spi_transfer_setup(struct rpc_spi *rpc,
>>> + struct spi_message *msg)
>>> +{
>>> + struct spi_transfer *t, xfer[4] = { };
>>> + u32 i, xfercnt, xferpos = 0;
>>> +
>>> + rpc->totalxferlen = 0;
>>> + rpc->xfer_dir = SPI_MEM_NO_DATA;
>>> +
>>> + list_for_each_entry(t, &msg->transfers, transfer_list) {
>>> + if (t->tx_buf) {
>>> + xfer[xferpos].tx_buf = t->tx_buf;
>>> + xfer[xferpos].tx_nbits = t->tx_nbits;
>>> + }
>>> +
>>> + if (t->rx_buf) {
>>> + xfer[xferpos].rx_buf = t->rx_buf;
>>> + xfer[xferpos].rx_nbits = t->rx_nbits;
>>> + }
>>> +
>>> + if (t->len) {
>>> + xfer[xferpos++].len = t->len;
>>> + rpc->totalxferlen += t->len;
>>> + }
>>> +
>>> + if (list_is_last(&t->transfer_list, &msg->transfers)) {
>>> + if (xferpos > 1 && t->rx_buf) {
>>> + rpc->xfer_dir = SPI_MEM_DATA_IN;
>>> + rpc->smcr = RPC_SMCR_SPIRE;
>>> + } else if (xferpos > 1 && t->tx_buf) {
>>
>> Why check 'xferpos' twice?
>>
>>> + rpc->xfer_dir = SPI_MEM_DATA_OUT;
>>> + rpc->smcr = RPC_SMCR_SPIWE;
>>> + }
>>> + }
>>> + }
>
> patch it to check 'xferpos' only one time.
> -------------------------------------------------------------
> if (list_is_last(&t->transfer_list, &msg->transfers)) {
> if (xferpos > 1) {
> if (tx->rx_buf) {
> rpc->xfer_dir = SPI_MEM_DATA_IN;
> rpc->smcr = RPC_SMCR_SPIRE;
> } else if (t->tx_buf) {
> rpc->xfer_dir = SPI_MEM_DATA_OUT;
> rpc->smcr = RPC_SMCR_SPIWE;
> }
> }
> }
> ----------------------------------------------------------
You got the idea but please reformat this properly..
[...]
>>
>>> + for (i = 0; i < xfer[1].len; i++)
>>> + rpc->addr |= (u32)((u8 *)xfer[1].tx_buf)[i]
>>> + << (8 * (xfer[1].len - i - 1));
>>
>> Ugh, you need get_unaligned_*()...
>
> for accessing a single byte quantity, ((u8 *)xfer[1].tx_buf)[i] ?
Ugh, never start a new line with an operator, lease it on a 1st, broken up line.
[...]
>>> +static const struct regmap_config rpc_spi_regmap_config = {
>>> + .reg_bits = 32,
>>> + .val_bits = 32,
>>> + .reg_stride = 4,
>>> + .fast_io = true,
>>> + .max_register = RPC_WBUF + RPC_WBUF_SIZE,
>>
>> Ugh... 0x8100/4 regs, of which the majority isn't used... :-/
Seriously, you don't use regmap for the write buffer anyway...
[...]
>> > +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);
>>
>> No spi_master_put() here?
>
> put_device() in spi_unregister_master().
Why call spi_master_put() in the probe() method's error path?
> 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.
>
> =====================================================================
Please consider sending patches from e.g. your GMail account to avoid this legelese
crap.
MBR, Sergei
Hello!
On 12/21/2018 01:46 PM, [email protected] wrote:
>> >>> +static ssize_t rpc_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
>> >>> + u64 offs, size_t len, const void *buf)
>> >>> +{
>> >>> + struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
>> >>> + int ret;
>> >>> +
>> >>> + if (WARN_ON(offs + desc->info.offset + len > U32_MAX))
>> >>> + return -EINVAL;
>> >>> +
>> >>> + if (WARN_ON(len > RPC_WBUF_SIZE))
>> >>> + return -EIO;
>> >>
>> >> Why not write 256 bytes and return w/that?
>> >
>> > in manual 62.3.13 Write Buffer Operation,
>> > transfer size to device is 256-byte unit.
>>
>> Why not write 256 bytes max and just return 256?
>>
>
> ?
> I don't get your point.
>
> here writes 256 byte each time and return 256 (len).
I mean not aborting the requests for >256 bytes right away (like you do) but
write only 256 bytes and return 256, not -EIO.
[...]
>> >>
>> >>> + for (i = 0; i < xfer[1].len; i++)
>> >>> + rpc->addr |= (u32)((u8 *)xfer[1].tx_buf)[i]
>> >>> + << (8 * (xfer[1].len - i - 1));
>> >>
>> >> Ugh, you need get_unaligned_*()...
>> >
>> > for accessing a single byte quantity, ((u8 *)xfer[1].tx_buf)[i] ?
>>
>> Ugh, never start a new line with an operator, lease it on a 1st,
Sorry -- leave, not lease.
>> broken up line.
>
> okay, patch it to:
>
> rpc->addr |= (u32)((u8 *)xfer[1].tx_buf)[i] <<
> (8 * (xfer[1].len - i - 1));
OK, thanks.
[...]
>> [...]
>> >> > +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);
>> >>
>> >> No spi_master_put() here?
>> >
>> > put_device() in spi_unregister_master().
>>
>> Why call spi_master_put() in the probe() method's error path?
>>
>
> called get_device() in spi_register_master() !
Hm, this is somewhat asymmetric...
> thanks & best regards,
> Mason
[...]
MBR, Sergei