[PATCH v2 0/5] spi: Add CSI support for Renesas RZ/V2M

[PATCH v2 0/5] spi: Add CSI support for Renesas RZ/V2M

[Fabrizio Castro]

Dear All,

This series is to add support for the Clocked Serial Interface (CSI)
IP found in the Renesas RZ/V2M SoC.

Thanks,
Fab

v2: edited list of include files in drivers/spi/spi-rzv2m-csi.c

Fabrizio Castro (5):
  spi: dt-bindings: Add bindings for RZ/V2M CSI
  clk: renesas: r9a09g011: Add CSI related clocks
  spi: Add support for Renesas CSI
  arm64: dts: renesas: r9a09g011: Add CSI nodes
  arm64: defconfig: Enable Renesas RZ/V2M CSI driver

 .../bindings/spi/renesas,rzv2m-csi.yaml       |  70 ++
 arch/arm64/boot/dts/renesas/r9a09g011.dtsi    |  28 +
 arch/arm64/configs/defconfig                  |   1 +
 drivers/clk/renesas/r9a09g011-cpg.c           |  15 +
 drivers/spi/Kconfig                           |   6 +
 drivers/spi/Makefile                          |   1 +
 drivers/spi/spi-rzv2m-csi.c                   | 667 ++++++++++++++++++
 7 files changed, 788 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/renesas,rzv2m-csi.yaml
 create mode 100644 drivers/spi/spi-rzv2m-csi.c

-- 
2.34.1

[PATCH v2 0/5] spi: Add CSI support for Renesas RZ/V2M

[Fabrizio Castro]

Dear All,

This series is to add support for the Clocked Serial Interface (CSI)
IP found in the Renesas RZ/V2M SoC.

Thanks,
Fab

v2: edited list of include files in drivers/spi/spi-rzv2m-csi.c

Fabrizio Castro (5):
  spi: dt-bindings: Add bindings for RZ/V2M CSI
  clk: renesas: r9a09g011: Add CSI related clocks
  spi: Add support for Renesas CSI
  arm64: dts: renesas: r9a09g011: Add CSI nodes
  arm64: defconfig: Enable Renesas RZ/V2M CSI driver

 .../bindings/spi/renesas,rzv2m-csi.yaml       |  70 ++
 arch/arm64/boot/dts/renesas/r9a09g011.dtsi    |  28 +
 arch/arm64/configs/defconfig                  |   1 +
 drivers/clk/renesas/r9a09g011-cpg.c           |  15 +
 drivers/spi/Kconfig                           |   6 +
 drivers/spi/Makefile                          |   1 +
 drivers/spi/spi-rzv2m-csi.c                   | 667 ++++++++++++++++++
 7 files changed, 788 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/renesas,rzv2m-csi.yaml
 create mode 100644 drivers/spi/spi-rzv2m-csi.c

-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/5] spi: dt-bindings: Add bindings for RZ/V2M CSI

[Fabrizio Castro]

Add dt-bindings for the CSI IP found inside the RZ/V2M SoC.

Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
---

v2: no changes

 .../bindings/spi/renesas,rzv2m-csi.yaml       | 70 +++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/renesas,rzv2m-csi.yaml

diff --git a/Documentation/devicetree/bindings/spi/renesas,rzv2m-csi.yaml b/Documentation/devicetree/bindings/spi/renesas,rzv2m-csi.yaml
new file mode 100644
index 000000000000..e59183e53690
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/renesas,rzv2m-csi.yaml
@@ -0,0 +1,70 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spi/renesas,rzv2m-csi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas RZ/V2M Clocked Serial Interface (CSI)
+
+maintainers:
+  - Fabrizio Castro <fabrizio.castro.jz@renesas.com>
+  - Geert Uytterhoeven <geert+renesas@glider.be>
+
+allOf:
+  - $ref: spi-controller.yaml#
+
+properties:
+  compatible:
+    const: renesas,rzv2m-csi
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: The clock used to generate the output clock (CSICLK)
+      - description: Internal clock to access the registers (PCLK)
+
+  clock-names:
+    items:
+      - const: csiclk
+      - const: pclk
+
+  resets:
+    maxItems: 1
+
+  power-domains:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - resets
+  - power-domains
+  - '#address-cells'
+  - '#size-cells'
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/r9a09g011-cpg.h>
+    csi4: spi@a4020200 {
+        compatible = "renesas,rzv2m-csi";
+        reg = <0xa4020200 0x80>;
+        interrupts = <GIC_SPI 230 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&cpg CPG_MOD R9A09G011_CSI4_CLK>,
+                 <&cpg CPG_MOD R9A09G011_CPERI_GRPH_PCLK>;
+        clock-names = "csiclk", "pclk";
+        resets = <&cpg R9A09G011_CSI_GPH_PRESETN>;
+        power-domains = <&cpg>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+    };
-- 
2.34.1

[PATCH v2 2/5] clk: renesas: r9a09g011: Add CSI related clocks

[Fabrizio Castro]

The Renesas RZ/V2M SoC comes with 6 CSI IPs (CSI0, CSI1, CSI2
CSI3, CSI4, and CSI5), however Linux is only allowed control
of CSI0 and CSI4.
CSI0 shares its reset and PCLK lines with CSI1, CSI2, and CSI3.
CSI4 shares its reset and PCLK lines with CSI5.

This commit adds support for the relevant clocks.

Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---

v2: no changes

 drivers/clk/renesas/r9a09g011-cpg.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/clk/renesas/r9a09g011-cpg.c b/drivers/clk/renesas/r9a09g011-cpg.c
index 3d06baf5061d..dda9f29dff33 100644
--- a/drivers/clk/renesas/r9a09g011-cpg.c
+++ b/drivers/clk/renesas/r9a09g011-cpg.c
@@ -28,6 +28,8 @@
 #define DIV_W		DDIV_PACK(0x328, 0, 3)
 
 #define SEL_B		SEL_PLL_PACK(0x214, 0, 1)
+#define SEL_CSI0	SEL_PLL_PACK(0x330, 0, 1)
+#define SEL_CSI4	SEL_PLL_PACK(0x330, 4, 1)
 #define SEL_D		SEL_PLL_PACK(0x214, 1, 1)
 #define SEL_E		SEL_PLL_PACK(0x214, 2, 1)
 #define SEL_SDI		SEL_PLL_PACK(0x300, 0, 1)
@@ -58,6 +60,8 @@ enum clk_ids {
 	CLK_DIV_W,
 	CLK_SEL_B,
 	CLK_SEL_B_D2,
+	CLK_SEL_CSI0,
+	CLK_SEL_CSI4,
 	CLK_SEL_D,
 	CLK_SEL_E,
 	CLK_SEL_SDI,
@@ -108,6 +112,7 @@ static const struct clk_div_table dtable_divw[] = {
 
 /* Mux clock tables */
 static const char * const sel_b[] = { ".main", ".divb" };
+static const char * const sel_csi[] = { ".main_24", ".main" };
 static const char * const sel_d[] = { ".main", ".divd" };
 static const char * const sel_e[] = { ".main", ".dive" };
 static const char * const sel_w[] = { ".main", ".divw" };
@@ -139,6 +144,8 @@ static const struct cpg_core_clk r9a09g011_core_clks[] __initconst = {
 	DEF_MUX_RO(".seld",	CLK_SEL_D,	SEL_D,		sel_d),
 	DEF_MUX_RO(".sele",	CLK_SEL_E,	SEL_E,		sel_e),
 	DEF_MUX(".selsdi",	CLK_SEL_SDI,	SEL_SDI,	sel_sdi),
+	DEF_MUX(".selcsi0",	CLK_SEL_CSI0,	SEL_CSI0,	sel_csi),
+	DEF_MUX(".selcsi4",	CLK_SEL_CSI4,	SEL_CSI4,	sel_csi),
 	DEF_MUX(".selw0",	CLK_SEL_W0,	SEL_W0,		sel_w),
 
 	DEF_FIXED(".selb_d2",	CLK_SEL_B_D2,	CLK_SEL_B,	1,	2),
@@ -196,8 +203,12 @@ static const struct rzg2l_mod_clk r9a09g011_mod_clks[] __initconst = {
 	DEF_MOD("pwm12_clk",	R9A09G011_PWM12_CLK,	 CLK_MAIN,     0x434, 8),
 	DEF_MOD("pwm13_clk",	R9A09G011_PWM13_CLK,	 CLK_MAIN,     0x434, 9),
 	DEF_MOD("pwm14_clk",	R9A09G011_PWM14_CLK,	 CLK_MAIN,     0x434, 10),
+	DEF_MOD("cperi_grpg",	R9A09G011_CPERI_GRPG_PCLK, CLK_SEL_E,  0x438, 0),
+	DEF_MOD("cperi_grph",	R9A09G011_CPERI_GRPH_PCLK, CLK_SEL_E,  0x438, 1),
 	DEF_MOD("urt_pclk",	R9A09G011_URT_PCLK,	 CLK_SEL_E,    0x438, 4),
 	DEF_MOD("urt0_clk",	R9A09G011_URT0_CLK,	 CLK_SEL_W0,   0x438, 5),
+	DEF_MOD("csi0_clk",	R9A09G011_CSI0_CLK,	 CLK_SEL_CSI0, 0x438, 8),
+	DEF_MOD("csi4_clk",	R9A09G011_CSI4_CLK,	 CLK_SEL_CSI4, 0x438, 12),
 	DEF_MOD("ca53",		R9A09G011_CA53_CLK,	 CLK_DIV_A,    0x448, 0),
 };
 
@@ -215,6 +226,8 @@ static const struct rzg2l_reset r9a09g011_resets[] = {
 	DEF_RST(R9A09G011_TIM_GPB_PRESETN,	0x614, 1),
 	DEF_RST(R9A09G011_TIM_GPC_PRESETN,	0x614, 2),
 	DEF_RST_MON(R9A09G011_PWM_GPF_PRESETN,	0x614, 5, 23),
+	DEF_RST_MON(R9A09G011_CSI_GPG_PRESETN,	0x614, 6, 22),
+	DEF_RST_MON(R9A09G011_CSI_GPH_PRESETN,	0x614, 7, 23),
 	DEF_RST(R9A09G011_IIC_GPA_PRESETN,	0x614, 8),
 	DEF_RST(R9A09G011_IIC_GPB_PRESETN,	0x614, 9),
 	DEF_RST_MON(R9A09G011_WDT0_PRESETN,	0x614, 12, 19),
@@ -225,6 +238,8 @@ static const unsigned int r9a09g011_crit_mod_clks[] __initconst = {
 	MOD_CLK_BASE + R9A09G011_CPERI_GRPB_PCLK,
 	MOD_CLK_BASE + R9A09G011_CPERI_GRPC_PCLK,
 	MOD_CLK_BASE + R9A09G011_CPERI_GRPF_PCLK,
+	MOD_CLK_BASE + R9A09G011_CPERI_GRPG_PCLK,
+	MOD_CLK_BASE + R9A09G011_CPERI_GRPH_PCLK,
 	MOD_CLK_BASE + R9A09G011_GIC_CLK,
 	MOD_CLK_BASE + R9A09G011_SYC_CNT_CLK,
 	MOD_CLK_BASE + R9A09G011_URT_PCLK,
-- 
2.34.1

[PATCH v2 3/5] spi: Add support for Renesas CSI

[Fabrizio Castro]

The RZ/V2M SoC comes with the Clocked Serial Interface (CSI)
IP, which is a master/slave SPI controller.

This commit adds a driver to support CSI master mode.

Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---

v2: edited includes in drivers/spi/spi-rzv2m-csi.c

 drivers/spi/Kconfig         |   6 +
 drivers/spi/Makefile        |   1 +
 drivers/spi/spi-rzv2m-csi.c | 667 ++++++++++++++++++++++++++++++++++++
 3 files changed, 674 insertions(+)
 create mode 100644 drivers/spi/spi-rzv2m-csi.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 14810d24733b..abbd1fb5fbc0 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -825,6 +825,12 @@ config SPI_RSPI
 	help
 	  SPI driver for Renesas RSPI and QSPI blocks.
 
+config SPI_RZV2M_CSI
+	tristate "Renesas RZV2M CSI controller"
+	depends on ARCH_RENESAS || COMPILE_TEST
+	help
+	  SPI driver for Renesas RZ/V2M Clocked Serial Interface (CSI)
+
 config SPI_QCOM_QSPI
 	tristate "QTI QSPI controller"
 	depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 28c4817a8a74..080c2c1b3ec1 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -113,6 +113,7 @@ obj-$(CONFIG_SPI_RB4XX)			+= spi-rb4xx.o
 obj-$(CONFIG_MACH_REALTEK_RTL)		+= spi-realtek-rtl.o
 obj-$(CONFIG_SPI_RPCIF)			+= spi-rpc-if.o
 obj-$(CONFIG_SPI_RSPI)			+= spi-rspi.o
+obj-$(CONFIG_SPI_RZV2M_CSI)		+= spi-rzv2m-csi.o
 obj-$(CONFIG_SPI_S3C64XX)		+= spi-s3c64xx.o
 obj-$(CONFIG_SPI_SC18IS602)		+= spi-sc18is602.o
 obj-$(CONFIG_SPI_SH)			+= spi-sh.o
diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c
new file mode 100644
index 000000000000..14ad65da930d
--- /dev/null
+++ b/drivers/spi/spi-rzv2m-csi.c
@@ -0,0 +1,667 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Renesas RZ/V2M Clocked Serial Interface (CSI) driver
+ *
+ * Copyright (C) 2023 Renesas Electronics Corporation
+ */
+
+#include <linux/clk.h>
+#include <linux/count_zeros.h>
+#include <linux/interrupt.h>
+#include <linux/iopoll.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/spi/spi.h>
+
+/* Registers */
+#define CSI_MODE		0x00	/* CSI mode control */
+#define CSI_CLKSEL		0x04	/* CSI clock select */
+#define CSI_CNT			0x08	/* CSI control */
+#define CSI_INT			0x0C	/* CSI interrupt status */
+#define CSI_IFIFOL		0x10	/* CSI receive FIFO level display */
+#define CSI_OFIFOL		0x14	/* CSI transmit FIFO level display */
+#define CSI_IFIFO		0x18	/* CSI receive window */
+#define CSI_OFIFO		0x1C	/* CSI transmit window */
+#define CSI_FIFOTRG		0x20	/* CSI FIFO trigger level */
+
+/* CSI_MODE */
+#define CSI_MODE_CSIE		BIT(7)
+#define CSI_MODE_TRMD		BIT(6)
+#define CSI_MODE_CCL		BIT(5)
+#define CSI_MODE_DIR		BIT(4)
+#define CSI_MODE_CSOT		BIT(0)
+
+#define CSI_MODE_SETUP		0x00000040
+
+/* CSI_CLKSEL */
+#define CSI_CLKSEL_CKP		BIT(17)
+#define CSI_CLKSEL_DAP		BIT(16)
+#define CSI_CLKSEL_SLAVE	BIT(15)
+#define CSI_CLKSEL_CKS		GENMASK(14, 1)
+
+/* CSI_CNT */
+#define CSI_CNT_CSIRST		BIT(28)
+#define CSI_CNT_R_TRGEN		BIT(19)
+#define CSI_CNT_UNDER_E		BIT(13)
+#define CSI_CNT_OVERF_E		BIT(12)
+#define CSI_CNT_TREND_E		BIT(9)
+#define CSI_CNT_CSIEND_E	BIT(8)
+#define CSI_CNT_T_TRGR_E	BIT(4)
+#define CSI_CNT_R_TRGR_E	BIT(0)
+
+/* CSI_INT */
+#define CSI_INT_UNDER		BIT(13)
+#define CSI_INT_OVERF		BIT(12)
+#define CSI_INT_TREND		BIT(9)
+#define CSI_INT_CSIEND		BIT(8)
+#define CSI_INT_T_TRGR		BIT(4)
+#define CSI_INT_R_TRGR		BIT(0)
+
+/* CSI_FIFOTRG */
+#define CSI_FIFOTRG_R_TRG       GENMASK(2, 0)
+
+#define CSI_FIFO_SIZE_BYTES	32
+#define CSI_FIFO_HALF_SIZE	16
+#define CSI_EN_DIS_TIMEOUT_US	100
+#define CSI_CKS_MAX		0x3FFF
+
+#define UNDERRUN_ERROR		BIT(0)
+#define OVERFLOW_ERROR		BIT(1)
+#define TX_TIMEOUT_ERROR	BIT(2)
+#define RX_TIMEOUT_ERROR	BIT(3)
+
+#define CSI_MAX_SPI_SCKO	8000000
+
+struct rzv2m_csi_priv {
+	void __iomem *base;
+	struct clk *csiclk;
+	struct clk *pclk;
+	struct device *dev;
+	struct spi_controller *controller;
+	const u8 *txbuf;
+	u8 *rxbuf;
+	int buffer_len;
+	int bytes_sent;
+	int bytes_received;
+	int bytes_to_transfer;
+	int words_to_transfer;
+	unsigned char bytes_per_word;
+	wait_queue_head_t wait;
+	u8 errors;
+	u32 status;
+};
+
+static const unsigned char x_trg[] = {
+	0, 1, 1, 2, 2, 2, 2, 3,
+	3, 3, 3, 3, 3, 3, 3, 4,
+	4, 4, 4, 4, 4, 4, 4, 4,
+	4, 4, 4, 4, 4, 4, 4, 5
+};
+
+static const unsigned char x_trg_words[] = {
+	1,  2,  2,  4,  4,  4,  4,  8,
+	8,  8,  8,  8,  8,  8,  8,  16,
+	16, 16, 16, 16, 16, 16, 16, 16,
+	16, 16, 16, 16, 16, 16, 16, 32
+};
+
+static void rzv2m_csi_reg_write_bit(const struct rzv2m_csi_priv *csi,
+				    int reg_offs, int bit_mask, u32 value)
+{
+	int nr_zeros;
+	u32 tmp;
+
+	nr_zeros = count_trailing_zeros(bit_mask);
+	value <<= nr_zeros;
+
+	tmp = (readl(csi->base + reg_offs) & ~bit_mask) | value;
+	writel(tmp, csi->base + reg_offs);
+}
+
+static int rzv2m_csi_sw_reset(struct rzv2m_csi_priv *csi, int assert)
+{
+	u32 reg;
+
+	rzv2m_csi_reg_write_bit(csi, CSI_CNT, CSI_CNT_CSIRST, assert);
+
+	if (assert) {
+		return readl_poll_timeout(csi->base + CSI_MODE, reg,
+					  !(reg & CSI_MODE_CSOT), 0,
+					  CSI_EN_DIS_TIMEOUT_US);
+	}
+
+	return 0;
+}
+
+static int rzv2m_csi_start_stop_operation(const struct rzv2m_csi_priv *csi,
+					  int enable, bool wait)
+{
+	u32 reg;
+
+	rzv2m_csi_reg_write_bit(csi, CSI_MODE, CSI_MODE_CSIE, enable);
+
+	if (!enable && wait)
+		return readl_poll_timeout(csi->base + CSI_MODE, reg,
+					  !(reg & CSI_MODE_CSOT), 0,
+					  CSI_EN_DIS_TIMEOUT_US);
+
+	return 0;
+}
+
+static int rzv2m_csi_fill_txfifo(struct rzv2m_csi_priv *csi)
+{
+	int i;
+
+	if (readl(csi->base + CSI_OFIFOL))
+		return -EIO;
+
+	if (csi->bytes_per_word == 2) {
+		u16 *buf = (u16 *)csi->txbuf;
+
+		for (i = 0; i < csi->words_to_transfer; i++)
+			writel(buf[i], csi->base + CSI_OFIFO);
+	} else {
+		u8 *buf = (u8 *)csi->txbuf;
+
+		for (i = 0; i < csi->words_to_transfer; i++)
+			writel(buf[i], csi->base + CSI_OFIFO);
+	}
+
+	csi->txbuf += csi->bytes_to_transfer;
+	csi->bytes_sent += csi->bytes_to_transfer;
+
+	return 0;
+}
+
+static int rzv2m_csi_read_rxfifo(struct rzv2m_csi_priv *csi)
+{
+	int i;
+
+	if (readl(csi->base + CSI_IFIFOL) != csi->bytes_to_transfer)
+		return -EIO;
+
+	if (csi->bytes_per_word == 2) {
+		u16 *buf = (u16 *)csi->rxbuf;
+
+		for (i = 0; i < csi->words_to_transfer; i++)
+			buf[i] = (u16)readl(csi->base + CSI_IFIFO);
+	} else {
+		u8 *buf = (u8 *)csi->rxbuf;
+
+		for (i = 0; i < csi->words_to_transfer; i++)
+			buf[i] = (u8)readl(csi->base + CSI_IFIFO);
+	}
+
+	csi->rxbuf += csi->bytes_to_transfer;
+	csi->bytes_received += csi->bytes_to_transfer;
+
+	return 0;
+}
+
+static inline void rzv2m_csi_calc_current_transfer(struct rzv2m_csi_priv *csi)
+{
+	int bytes_transferred = max_t(int, csi->bytes_received, csi->bytes_sent);
+	int bytes_remaining = csi->buffer_len - bytes_transferred;
+	int to_transfer;
+
+	if (csi->txbuf)
+		/*
+		 * Leaving a little bit of headroom in the FIFOs makes it very
+		 * hard to raise an overflow error (which is only possible
+		 * when IP transmits and receives at the same time).
+		 */
+		to_transfer = min_t(int, CSI_FIFO_HALF_SIZE, bytes_remaining);
+	else
+		to_transfer = min_t(int, CSI_FIFO_SIZE_BYTES, bytes_remaining);
+
+	if (csi->bytes_per_word == 2)
+		to_transfer >>= 1;
+
+	/*
+	 * We can only choose a trigger level from a predefined set of values.
+	 * This will pick a value that is the greatest possible integer that's
+	 * less than or equal to the number of bytes we need to transfer.
+	 * This may result in multiple smaller transfers.
+	 */
+	csi->words_to_transfer = x_trg_words[to_transfer - 1];
+
+	if (csi->bytes_per_word == 2)
+		csi->bytes_to_transfer = csi->words_to_transfer << 1;
+	else
+		csi->bytes_to_transfer = csi->words_to_transfer;
+}
+
+static inline void rzv2m_csi_set_rx_fifo_trigger_level(struct rzv2m_csi_priv *csi)
+{
+	rzv2m_csi_reg_write_bit(csi, CSI_FIFOTRG, CSI_FIFOTRG_R_TRG,
+				x_trg[csi->words_to_transfer - 1]);
+}
+
+static inline void rzv2m_csi_enable_rx_trigger(struct rzv2m_csi_priv *csi,
+					       bool enable)
+{
+	rzv2m_csi_reg_write_bit(csi, CSI_CNT, CSI_CNT_R_TRGEN, enable);
+}
+
+static void rzv2m_csi_disable_irqs(const struct rzv2m_csi_priv *csi,
+				   u32 enable_bits)
+{
+	u32 cnt = readl(csi->base + CSI_CNT);
+
+	writel(cnt & ~enable_bits, csi->base + CSI_CNT);
+}
+
+static void rzv2m_csi_disable_all_irqs(struct rzv2m_csi_priv *csi)
+{
+	rzv2m_csi_disable_irqs(csi, CSI_CNT_R_TRGR_E | CSI_CNT_T_TRGR_E |
+			       CSI_CNT_CSIEND_E | CSI_CNT_TREND_E |
+			       CSI_CNT_OVERF_E | CSI_CNT_UNDER_E);
+}
+
+static inline void rzv2m_csi_clear_irqs(struct rzv2m_csi_priv *csi, u32 irqs)
+{
+	writel(irqs, csi->base + CSI_INT);
+}
+
+static void rzv2m_csi_clear_all_irqs(struct rzv2m_csi_priv *csi)
+{
+	rzv2m_csi_clear_irqs(csi, CSI_INT_UNDER | CSI_INT_OVERF |
+			     CSI_INT_TREND | CSI_INT_CSIEND |  CSI_INT_T_TRGR |
+			     CSI_INT_R_TRGR);
+}
+
+static void rzv2m_csi_enable_irqs(struct rzv2m_csi_priv *csi, u32 enable_bits)
+{
+	u32 cnt = readl(csi->base + CSI_CNT);
+
+	writel(cnt | enable_bits, csi->base + CSI_CNT);
+}
+
+static int rzv2m_csi_wait_for_interrupt(struct rzv2m_csi_priv *csi,
+					u32 wait_mask, u32 enable_bits)
+{
+	int ret;
+
+	rzv2m_csi_enable_irqs(csi, enable_bits);
+
+	ret = wait_event_timeout(csi->wait,
+				 ((csi->status & wait_mask) == wait_mask) ||
+				 csi->errors, HZ);
+
+	rzv2m_csi_disable_irqs(csi, enable_bits);
+
+	if (csi->errors)
+		return -EIO;
+
+	if (!ret)
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+static int rzv2m_csi_wait_for_tx_empty(struct rzv2m_csi_priv *csi)
+{
+	int ret;
+
+	if (readl(csi->base + CSI_OFIFOL) == 0)
+		return 0;
+
+	ret = rzv2m_csi_wait_for_interrupt(csi, CSI_INT_TREND, CSI_CNT_TREND_E);
+
+	if (ret == -ETIMEDOUT)
+		csi->errors |= TX_TIMEOUT_ERROR;
+
+	return ret;
+}
+
+static inline int rzv2m_csi_wait_for_rx_ready(struct rzv2m_csi_priv *csi)
+{
+	int ret;
+
+	if (readl(csi->base + CSI_IFIFOL) == csi->bytes_to_transfer)
+		return 0;
+
+	ret = rzv2m_csi_wait_for_interrupt(csi, CSI_INT_R_TRGR,
+					   CSI_CNT_R_TRGR_E);
+
+	if (ret == -ETIMEDOUT)
+		csi->errors |= RX_TIMEOUT_ERROR;
+
+	return ret;
+}
+
+static irqreturn_t rzv2m_csi_irq_handler(int irq, void *data)
+{
+	struct rzv2m_csi_priv *csi = (struct rzv2m_csi_priv *)data;
+
+	csi->status = readl(csi->base + CSI_INT);
+	rzv2m_csi_disable_irqs(csi, csi->status);
+
+	if (csi->status & CSI_INT_OVERF)
+		csi->errors |= OVERFLOW_ERROR;
+	if (csi->status & CSI_INT_UNDER)
+		csi->errors |= UNDERRUN_ERROR;
+
+	wake_up(&csi->wait);
+
+	return IRQ_HANDLED;
+}
+
+static void rzv2m_csi_setup_clock(struct rzv2m_csi_priv *csi, u32 spi_hz)
+{
+	unsigned long csiclk_rate = clk_get_rate(csi->csiclk);
+	unsigned long pclk_rate = clk_get_rate(csi->pclk);
+	unsigned long csiclk_rate_limit = pclk_rate >> 1;
+	u32 cks;
+
+	/*
+	 * There is a restriction on the frequency of CSICLK, it has to be <=
+	 * PCLK / 2.
+	 */
+	if (csiclk_rate > csiclk_rate_limit) {
+		clk_set_rate(csi->csiclk, csiclk_rate >> 1);
+		csiclk_rate = clk_get_rate(csi->csiclk);
+	} else if ((csiclk_rate << 1) <= csiclk_rate_limit) {
+		clk_set_rate(csi->csiclk, csiclk_rate << 1);
+		csiclk_rate = clk_get_rate(csi->csiclk);
+	}
+
+	spi_hz = spi_hz > CSI_MAX_SPI_SCKO ? CSI_MAX_SPI_SCKO : spi_hz;
+
+	cks = DIV_ROUND_UP(csiclk_rate, spi_hz << 1);
+	if (cks > CSI_CKS_MAX)
+		cks = CSI_CKS_MAX;
+
+	dev_dbg(csi->dev, "SPI clk rate is %ldHz\n", csiclk_rate / (cks << 1));
+
+	rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_CKS, cks);
+}
+
+static void rzv2m_csi_setup_operating_mode(struct rzv2m_csi_priv *csi,
+					   struct spi_transfer *t)
+{
+	if (t->rx_buf && !t->tx_buf)
+		/* Reception-only mode */
+		rzv2m_csi_reg_write_bit(csi, CSI_MODE, CSI_MODE_TRMD, 0);
+	else
+		/* Send and receive mode */
+		rzv2m_csi_reg_write_bit(csi, CSI_MODE, CSI_MODE_TRMD, 1);
+
+	csi->bytes_per_word = t->bits_per_word / 8;
+	rzv2m_csi_reg_write_bit(csi, CSI_MODE, CSI_MODE_CCL,
+				csi->bytes_per_word == 2);
+}
+
+static int rzv2m_csi_setup(struct spi_device *spi)
+{
+	struct rzv2m_csi_priv *csi = spi_controller_get_devdata(spi->controller);
+	int ret;
+
+	rzv2m_csi_sw_reset(csi, 0);
+
+	writel(CSI_MODE_SETUP, csi->base + CSI_MODE);
+
+	/* Setup clock polarity and phase timing */
+	rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_CKP,
+				!(spi->mode & SPI_CPOL));
+	rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_DAP,
+				!(spi->mode & SPI_CPHA));
+
+	/* Setup serial data order */
+	rzv2m_csi_reg_write_bit(csi, CSI_MODE, CSI_MODE_DIR,
+				!!(spi->mode & SPI_LSB_FIRST));
+
+	/* Set the operation mode as master */
+	rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_SLAVE, 0);
+
+	/* Give the IP a SW reset */
+	ret = rzv2m_csi_sw_reset(csi, 1);
+	if (ret)
+		return ret;
+	rzv2m_csi_sw_reset(csi, 0);
+
+	/*
+	 * We need to enable the communication so that the clock will settle
+	 * for the right polarity before enabling the CS.
+	 */
+	rzv2m_csi_start_stop_operation(csi, 1, false);
+	udelay(10);
+	rzv2m_csi_start_stop_operation(csi, 0, false);
+
+	return 0;
+}
+
+static int rzv2m_csi_pio_transfer(struct rzv2m_csi_priv *csi)
+{
+	bool tx_completed = csi->txbuf ? false : true;
+	bool rx_completed = csi->rxbuf ? false : true;
+	int ret = 0;
+
+	/* Make sure the TX FIFO is empty */
+	writel(0, csi->base + CSI_OFIFOL);
+
+	csi->bytes_sent = 0;
+	csi->bytes_received = 0;
+	csi->errors = 0;
+
+	rzv2m_csi_disable_all_irqs(csi);
+	rzv2m_csi_clear_all_irqs(csi);
+	rzv2m_csi_enable_rx_trigger(csi, true);
+
+	while (!tx_completed || !rx_completed) {
+		/*
+		 * Decide how many words we are going to transfer during
+		 * this cycle (for both TX and RX), then set the RX FIFO trigger
+		 * level accordingly. No need to set a trigger level for the
+		 * TX FIFO, as this IP comes with an interrupt that fires when
+		 * the TX FIFO is empty.
+		 */
+		rzv2m_csi_calc_current_transfer(csi);
+		rzv2m_csi_set_rx_fifo_trigger_level(csi);
+
+		rzv2m_csi_enable_irqs(csi, CSI_INT_OVERF | CSI_INT_UNDER);
+
+		/* Make sure the RX FIFO is empty */
+		writel(0, csi->base + CSI_IFIFOL);
+
+		writel(readl(csi->base + CSI_INT), csi->base + CSI_INT);
+		csi->status = 0;
+
+		rzv2m_csi_start_stop_operation(csi, 1, false);
+
+		/* TX */
+		if (csi->txbuf) {
+			ret = rzv2m_csi_fill_txfifo(csi);
+			if (ret)
+				break;
+
+			ret = rzv2m_csi_wait_for_tx_empty(csi);
+			if (ret)
+				break;
+
+			if (csi->bytes_sent == csi->buffer_len)
+				tx_completed = true;
+		}
+
+		/*
+		 * Make sure the RX FIFO contains the desired number of words.
+		 * We then either flush its content, or we copy it onto
+		 * csi->rxbuf.
+		 */
+		ret = rzv2m_csi_wait_for_rx_ready(csi);
+		if (ret)
+			break;
+
+		/* RX */
+		if (csi->rxbuf) {
+			rzv2m_csi_start_stop_operation(csi, 0, false);
+
+			ret = rzv2m_csi_read_rxfifo(csi);
+			if (ret)
+				break;
+
+			if (csi->bytes_received == csi->buffer_len)
+				rx_completed = true;
+		}
+
+		ret = rzv2m_csi_start_stop_operation(csi, 0, true);
+		if (ret)
+			goto pio_quit;
+
+		if (csi->errors) {
+			ret = -EIO;
+			goto pio_quit;
+		}
+	}
+
+	rzv2m_csi_start_stop_operation(csi, 0, true);
+
+pio_quit:
+	rzv2m_csi_disable_all_irqs(csi);
+	rzv2m_csi_enable_rx_trigger(csi, false);
+	rzv2m_csi_clear_all_irqs(csi);
+
+	return ret;
+}
+
+static int rzv2m_csi_transfer_one(struct spi_controller *controller,
+				  struct spi_device *spi,
+				  struct spi_transfer *transfer)
+{
+	struct rzv2m_csi_priv *csi = spi_controller_get_devdata(controller);
+	struct device *dev = csi->dev;
+	int ret;
+
+	csi->txbuf = transfer->tx_buf;
+	csi->rxbuf = transfer->rx_buf;
+	csi->buffer_len = transfer->len;
+
+	rzv2m_csi_setup_operating_mode(csi, transfer);
+
+	rzv2m_csi_setup_clock(csi, transfer->speed_hz);
+
+	ret = rzv2m_csi_pio_transfer(csi);
+	if (ret) {
+		if (csi->errors & UNDERRUN_ERROR)
+			dev_err(dev, "Underrun error\n");
+		if (csi->errors & OVERFLOW_ERROR)
+			dev_err(dev, "Overflow error\n");
+		if (csi->errors & TX_TIMEOUT_ERROR)
+			dev_err(dev, "TX timeout error\n");
+		if (csi->errors & RX_TIMEOUT_ERROR)
+			dev_err(dev, "RX timeout error\n");
+	}
+
+	return ret;
+}
+
+static int rzv2m_csi_probe(struct platform_device *pdev)
+{
+	struct spi_controller *controller;
+	struct device *dev = &pdev->dev;
+	struct rzv2m_csi_priv *csi;
+	struct reset_control *rstc;
+	int irq;
+	int ret;
+
+	controller = devm_spi_alloc_master(dev, sizeof(*csi));
+	if (!controller)
+		return -ENOMEM;
+
+	csi = spi_controller_get_devdata(controller);
+	platform_set_drvdata(pdev, csi);
+
+	csi->dev = dev;
+	csi->controller = controller;
+
+	csi->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(csi->base))
+		return PTR_ERR(csi->base);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	csi->csiclk = devm_clk_get(dev, "csiclk");
+	if (IS_ERR(csi->csiclk))
+		return dev_err_probe(dev, PTR_ERR(csi->csiclk),
+				     "could not get csiclk\n");
+
+	csi->pclk = devm_clk_get(dev, "pclk");
+	if (IS_ERR(csi->pclk))
+		return dev_err_probe(dev, PTR_ERR(csi->pclk),
+				     "could not get pclk\n");
+
+	rstc = devm_reset_control_get_shared(dev, NULL);
+	if (IS_ERR(rstc))
+		return dev_err_probe(dev, PTR_ERR(rstc), "Missing reset ctrl\n");
+
+	init_waitqueue_head(&csi->wait);
+
+	controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST;
+	controller->dev.of_node = pdev->dev.of_node;
+	controller->bits_per_word_mask = SPI_BPW_MASK(16) | SPI_BPW_MASK(8);
+	controller->setup = rzv2m_csi_setup;
+	controller->transfer_one = rzv2m_csi_transfer_one;
+	controller->use_gpio_descriptors = true;
+
+	ret = devm_request_irq(dev, irq, rzv2m_csi_irq_handler, 0,
+			       dev_name(dev), csi);
+	if (ret)
+		return dev_err_probe(dev, ret, "cannot request IRQ\n");
+
+	/*
+	 * The reset also affects other HW that is not under the control
+	 * of Linux. Therefore, all we can do is make sure the reset is
+	 * deasserted.
+	 */
+	reset_control_deassert(rstc);
+
+	/* Make sure the IP is in SW reset state */
+	ret = rzv2m_csi_sw_reset(csi, 1);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(csi->csiclk);
+	if (ret)
+		return dev_err_probe(dev, ret, "could not enable csiclk\n");
+
+	ret = spi_register_controller(controller);
+	if (ret) {
+		clk_disable_unprepare(csi->csiclk);
+		return dev_err_probe(dev, ret, "register controller failed\n");
+	}
+
+	return 0;
+}
+
+static int rzv2m_csi_remove(struct platform_device *pdev)
+{
+	struct rzv2m_csi_priv *csi = platform_get_drvdata(pdev);
+
+	spi_unregister_controller(csi->controller);
+	rzv2m_csi_sw_reset(csi, 1);
+	clk_disable_unprepare(csi->csiclk);
+
+	return 0;
+}
+
+static const struct of_device_id rzv2m_csi_match[] = {
+	{ .compatible = "renesas,rzv2m-csi" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rzv2m_csi_match);
+
+static struct platform_driver rzv2m_csi_drv = {
+	.probe = rzv2m_csi_probe,
+	.remove = rzv2m_csi_remove,
+	.driver = {
+		.name = "rzv2m_csi",
+		.of_match_table = rzv2m_csi_match,
+	},
+};
+module_platform_driver(rzv2m_csi_drv);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Fabrizio Castro <castro.fabrizio.jz@renesas.com>");
+MODULE_DESCRIPTION("Clocked Serial Interface Driver");
-- 
2.34.1

[PATCH v2 4/5] arm64: dts: renesas: r9a09g011: Add CSI nodes

[Fabrizio Castro]

The Renesas RZ/V2M comes with 6 Clocked Serial Interface (CSI)
IPs (CSI0, CSI1, CSI2, CSI3, CSI4, CSI5), but Linux is only
allowed access to CSI0 and CSI4.

This commit adds SoC specific device tree support for CSI0 and
CSI4.

Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---

v2: no changes

 arch/arm64/boot/dts/renesas/r9a09g011.dtsi | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r9a09g011.dtsi b/arch/arm64/boot/dts/renesas/r9a09g011.dtsi
index 46d67b200a66..33f2ecf42441 100644
--- a/arch/arm64/boot/dts/renesas/r9a09g011.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a09g011.dtsi
@@ -236,6 +236,34 @@ sys: system-controller@a3f03000 {
 			reg = <0 0xa3f03000 0 0x400>;
 		};
 
+		csi0: spi@a4020000 {
+			compatible = "renesas,rzv2m-csi";
+			reg = <0 0xa4020000 0 0x80>;
+			interrupts = <GIC_SPI 226 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD R9A09G011_CSI0_CLK>,
+				 <&cpg CPG_MOD R9A09G011_CPERI_GRPG_PCLK>;
+			clock-names = "csiclk", "pclk";
+			resets = <&cpg R9A09G011_CSI_GPG_PRESETN>;
+			power-domains = <&cpg>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+		};
+
+		csi4: spi@a4020200 {
+			compatible = "renesas,rzv2m-csi";
+			reg = <0 0xa4020200 0 0x80>;
+			interrupts = <GIC_SPI 230 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD R9A09G011_CSI4_CLK>,
+				 <&cpg CPG_MOD R9A09G011_CPERI_GRPH_PCLK>;
+			clock-names = "csiclk", "pclk";
+			resets = <&cpg R9A09G011_CSI_GPH_PRESETN>;
+			power-domains = <&cpg>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+		};
+
 		i2c0: i2c@a4030000 {
 			#address-cells = <1>;
 			#size-cells = <0>;
-- 
2.34.1

[PATCH v2 5/5] arm64: defconfig: Enable Renesas RZ/V2M CSI driver

[Fabrizio Castro]

Enable CSI driver support for Renesas RZ/V2M based platforms.

Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---

v2: no changes

 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index a24609e14d50..5bde48869ad0 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -526,6 +526,7 @@ CONFIG_SPI_PL022=y
 CONFIG_SPI_ROCKCHIP=y
 CONFIG_SPI_RPCIF=m
 CONFIG_SPI_RSPI=m
+CONFIG_SPI_RZV2M_CSI=m
 CONFIG_SPI_QCOM_QSPI=m
 CONFIG_SPI_QUP=y
 CONFIG_SPI_QCOM_GENI=m
-- 
2.34.1

[PATCH v2 5/5] arm64: defconfig: Enable Renesas RZ/V2M CSI driver

[Fabrizio Castro]

Enable CSI driver support for Renesas RZ/V2M based platforms.

Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---

v2: no changes

 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index a24609e14d50..5bde48869ad0 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -526,6 +526,7 @@ CONFIG_SPI_PL022=y
 CONFIG_SPI_ROCKCHIP=y
 CONFIG_SPI_RPCIF=m
 CONFIG_SPI_RSPI=m
+CONFIG_SPI_RZV2M_CSI=m
 CONFIG_SPI_QCOM_QSPI=m
 CONFIG_SPI_QUP=y
 CONFIG_SPI_QCOM_GENI=m
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/5] spi: Add CSI support for Renesas RZ/V2M

[Mark Brown]

On Thu, 22 Jun 2023 12:33:36 +0100, Fabrizio Castro wrote:
> This series is to add support for the Clocked Serial Interface (CSI)
> IP found in the Renesas RZ/V2M SoC.
> 
> Thanks,
> Fab
> 
> v2: edited list of include files in drivers/spi/spi-rzv2m-csi.c
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/5] spi: dt-bindings: Add bindings for RZ/V2M CSI
      commit: db63e7ad2895409f78a04f331f781baa7a879dd7
[2/5] clk: renesas: r9a09g011: Add CSI related clocks
      commit: 7c78eb3e5d30eaa217cecaa32711e41cd849d498
[3/5] spi: Add support for Renesas CSI
      commit: dcf92036cb3e1b7bf3472109e4290a0937b270dd
[4/5] arm64: dts: renesas: r9a09g011: Add CSI nodes
      commit: ef643c6b57020ee279d18636d9d967ee048dbffa
[5/5] arm64: defconfig: Enable Renesas RZ/V2M CSI driver
      commit: dfbd12ae0e7c761e07369f5a2d55fe06eb54ad31

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH v2 0/5] spi: Add CSI support for Renesas RZ/V2M

[Mark Brown]

On Thu, 22 Jun 2023 12:33:36 +0100, Fabrizio Castro wrote:
> This series is to add support for the Clocked Serial Interface (CSI)
> IP found in the Renesas RZ/V2M SoC.
> 
> Thanks,
> Fab
> 
> v2: edited list of include files in drivers/spi/spi-rzv2m-csi.c
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/5] spi: dt-bindings: Add bindings for RZ/V2M CSI
      commit: db63e7ad2895409f78a04f331f781baa7a879dd7
[2/5] clk: renesas: r9a09g011: Add CSI related clocks
      commit: 7c78eb3e5d30eaa217cecaa32711e41cd849d498
[3/5] spi: Add support for Renesas CSI
      commit: dcf92036cb3e1b7bf3472109e4290a0937b270dd
[4/5] arm64: dts: renesas: r9a09g011: Add CSI nodes
      commit: ef643c6b57020ee279d18636d9d967ee048dbffa
[5/5] arm64: defconfig: Enable Renesas RZ/V2M CSI driver
      commit: dfbd12ae0e7c761e07369f5a2d55fe06eb54ad31

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/5] spi: Add CSI support for Renesas RZ/V2M

[Geert Uytterhoeven]

Hi Mark,

On Fri, Jun 23, 2023 at 2:32 AM Mark Brown <broonie@kernel.org> wrote:
> On Thu, 22 Jun 2023 12:33:36 +0100, Fabrizio Castro wrote:
> > This series is to add support for the Clocked Serial Interface (CSI)
> > IP found in the Renesas RZ/V2M SoC.
> >
> > Thanks,
> > Fab
> >
> > v2: edited list of include files in drivers/spi/spi-rzv2m-csi.c
> >
> > [...]
>
> Applied to
>
>    https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
>
> Thanks!
>
> [1/5] spi: dt-bindings: Add bindings for RZ/V2M CSI
>       commit: db63e7ad2895409f78a04f331f781baa7a879dd7
> [2/5] clk: renesas: r9a09g011: Add CSI related clocks
>       commit: 7c78eb3e5d30eaa217cecaa32711e41cd849d498
> [3/5] spi: Add support for Renesas CSI
>       commit: dcf92036cb3e1b7bf3472109e4290a0937b270dd
> [4/5] arm64: dts: renesas: r9a09g011: Add CSI nodes
>       commit: ef643c6b57020ee279d18636d9d967ee048dbffa
> [5/5] arm64: defconfig: Enable Renesas RZ/V2M CSI driver
>       commit: dfbd12ae0e7c761e07369f5a2d55fe06eb54ad31

I hoped this would have been a bug in b4 thanks, but unfortunately it
is not.

Please do not apply unreviewed clock, DTS, and defconfig patches to
your tree.  These are intended to go upstream through the renesas-clk
and clk, renesas-dt and soc, resp. renesas-defconfig and soc trees.

Thanks for your understanding!

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH v2 0/5] spi: Add CSI support for Renesas RZ/V2M

[Geert Uytterhoeven]

Hi Mark,

On Fri, Jun 23, 2023 at 2:32 AM Mark Brown <broonie@kernel.org> wrote:
> On Thu, 22 Jun 2023 12:33:36 +0100, Fabrizio Castro wrote:
> > This series is to add support for the Clocked Serial Interface (CSI)
> > IP found in the Renesas RZ/V2M SoC.
> >
> > Thanks,
> > Fab
> >
> > v2: edited list of include files in drivers/spi/spi-rzv2m-csi.c
> >
> > [...]
>
> Applied to
>
>    https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
>
> Thanks!
>
> [1/5] spi: dt-bindings: Add bindings for RZ/V2M CSI
>       commit: db63e7ad2895409f78a04f331f781baa7a879dd7
> [2/5] clk: renesas: r9a09g011: Add CSI related clocks
>       commit: 7c78eb3e5d30eaa217cecaa32711e41cd849d498
> [3/5] spi: Add support for Renesas CSI
>       commit: dcf92036cb3e1b7bf3472109e4290a0937b270dd
> [4/5] arm64: dts: renesas: r9a09g011: Add CSI nodes
>       commit: ef643c6b57020ee279d18636d9d967ee048dbffa
> [5/5] arm64: defconfig: Enable Renesas RZ/V2M CSI driver
>       commit: dfbd12ae0e7c761e07369f5a2d55fe06eb54ad31

I hoped this would have been a bug in b4 thanks, but unfortunately it
is not.

Please do not apply unreviewed clock, DTS, and defconfig patches to
your tree.  These are intended to go upstream through the renesas-clk
and clk, renesas-dt and soc, resp. renesas-defconfig and soc trees.

Thanks for your understanding!

Gr{oetje,eeting}s,

                        Geert

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

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/5] spi: Add CSI support for Renesas RZ/V2M

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1278 bytes --]

On Fri, Jun 23, 2023 at 08:49:05AM +0200, Geert Uytterhoeven wrote:
> On Fri, Jun 23, 2023 at 2:32 AM Mark Brown <broonie@kernel.org> wrote:

> > [1/5] spi: dt-bindings: Add bindings for RZ/V2M CSI
> >       commit: db63e7ad2895409f78a04f331f781baa7a879dd7
> > [2/5] clk: renesas: r9a09g011: Add CSI related clocks
> >       commit: 7c78eb3e5d30eaa217cecaa32711e41cd849d498
> > [3/5] spi: Add support for Renesas CSI
> >       commit: dcf92036cb3e1b7bf3472109e4290a0937b270dd
> > [4/5] arm64: dts: renesas: r9a09g011: Add CSI nodes
> >       commit: ef643c6b57020ee279d18636d9d967ee048dbffa
> > [5/5] arm64: defconfig: Enable Renesas RZ/V2M CSI driver
> >       commit: dfbd12ae0e7c761e07369f5a2d55fe06eb54ad31
> 
> I hoped this would have been a bug in b4 thanks, but unfortunately it
> is not.
> 
> Please do not apply unreviewed clock, DTS, and defconfig patches to
> your tree.  These are intended to go upstream through the renesas-clk
> and clk, renesas-dt and soc, resp. renesas-defconfig and soc trees.

Sorry, the series was only partially copied to me so it wasn't very
visible that there were other patches - I just saw a simple 2 patch
series in my inbox and it's not terribly visible in the rest of the
process that there's more patches.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/5] spi: Add CSI support for Renesas RZ/V2M

[Mark Brown]

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

On Fri, Jun 23, 2023 at 08:49:05AM +0200, Geert Uytterhoeven wrote:
> On Fri, Jun 23, 2023 at 2:32 AM Mark Brown <broonie@kernel.org> wrote:

> > [1/5] spi: dt-bindings: Add bindings for RZ/V2M CSI
> >       commit: db63e7ad2895409f78a04f331f781baa7a879dd7
> > [2/5] clk: renesas: r9a09g011: Add CSI related clocks
> >       commit: 7c78eb3e5d30eaa217cecaa32711e41cd849d498
> > [3/5] spi: Add support for Renesas CSI
> >       commit: dcf92036cb3e1b7bf3472109e4290a0937b270dd
> > [4/5] arm64: dts: renesas: r9a09g011: Add CSI nodes
> >       commit: ef643c6b57020ee279d18636d9d967ee048dbffa
> > [5/5] arm64: defconfig: Enable Renesas RZ/V2M CSI driver
> >       commit: dfbd12ae0e7c761e07369f5a2d55fe06eb54ad31
> 
> I hoped this would have been a bug in b4 thanks, but unfortunately it
> is not.
> 
> Please do not apply unreviewed clock, DTS, and defconfig patches to
> your tree.  These are intended to go upstream through the renesas-clk
> and clk, renesas-dt and soc, resp. renesas-defconfig and soc trees.

Sorry, the series was only partially copied to me so it wasn't very
visible that there were other patches - I just saw a simple 2 patch
series in my inbox and it's not terribly visible in the rest of the
process that there's more patches.

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

Re: (subset) [PATCH v2 0/5] spi: Add CSI support for Renesas RZ/V2M

[Mark Brown]

On Thu, 22 Jun 2023 12:33:36 +0100, Fabrizio Castro wrote:
> This series is to add support for the Clocked Serial Interface (CSI)
> IP found in the Renesas RZ/V2M SoC.
> 
> Thanks,
> Fab
> 
> v2: edited list of include files in drivers/spi/spi-rzv2m-csi.c
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/5] spi: dt-bindings: Add bindings for RZ/V2M CSI
      commit: db63e7ad2895409f78a04f331f781baa7a879dd7
[3/5] spi: Add support for Renesas CSI
      commit: dcf92036cb3e1b7bf3472109e4290a0937b270dd

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: (subset) [PATCH v2 0/5] spi: Add CSI support for Renesas RZ/V2M

[Mark Brown]

On Thu, 22 Jun 2023 12:33:36 +0100, Fabrizio Castro wrote:
> This series is to add support for the Clocked Serial Interface (CSI)
> IP found in the Renesas RZ/V2M SoC.
> 
> Thanks,
> Fab
> 
> v2: edited list of include files in drivers/spi/spi-rzv2m-csi.c
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/5] spi: dt-bindings: Add bindings for RZ/V2M CSI
      commit: db63e7ad2895409f78a04f331f781baa7a879dd7
[3/5] spi: Add support for Renesas CSI
      commit: dcf92036cb3e1b7bf3472109e4290a0937b270dd

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/5] spi: dt-bindings: Add bindings for RZ/V2M CSI

[Geert Uytterhoeven]

On Thu, Jun 22, 2023 at 1:34 PM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
> Add dt-bindings for the CSI IP found inside the RZ/V2M SoC.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>
> v2: no changes

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH v2 2/5] clk: renesas: r9a09g011: Add CSI related clocks

[Geert Uytterhoeven]

Hi Fabrizio,

Thanks for your patch!

On Thu, Jun 22, 2023 at 1:34 PM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
> The Renesas RZ/V2M SoC comes with 6 CSI IPs (CSI0, CSI1, CSI2
> CSI3, CSI4, and CSI5), however Linux is only allowed control
> of CSI0 and CSI4.
> CSI0 shares its reset and PCLK lines with CSI1, CSI2, and CSI3.
> CSI4 shares its reset and PCLK lines with CSI5.

That sounds like a marvelous idea.... :-(

> This commit adds support for the relevant clocks.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> ---
>
> v2: no changes

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-clk-for-v6.6.

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH v2 3/5] spi: Add support for Renesas CSI

[Geert Uytterhoeven]

Hi Fabrizio,

On Thu, Jun 22, 2023 at 1:34 PM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
> The RZ/V2M SoC comes with the Clocked Serial Interface (CSI)
> IP, which is a master/slave SPI controller.
>
> This commit adds a driver to support CSI master mode.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> ---
>
> v2: edited includes in drivers/spi/spi-rzv2m-csi.c

Thanks for your patch!

> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -825,6 +825,12 @@ config SPI_RSPI
>         help
>           SPI driver for Renesas RSPI and QSPI blocks.
>
> +config SPI_RZV2M_CSI
> +       tristate "Renesas RZV2M CSI controller"

RZ/V2M (patch sent)

> +       depends on ARCH_RENESAS || COMPILE_TEST
> +       help
> +         SPI driver for Renesas RZ/V2M Clocked Serial Interface (CSI)
> +
>  config SPI_QCOM_QSPI
>         tristate "QTI QSPI controller"
>         depends on ARCH_QCOM || COMPILE_TEST

> --- /dev/null
> +++ b/drivers/spi/spi-rzv2m-csi.c
> @@ -0,0 +1,667 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Renesas RZ/V2M Clocked Serial Interface (CSI) driver
> + *
> + * Copyright (C) 2023 Renesas Electronics Corporation
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/count_zeros.h>
> +#include <linux/interrupt.h>
> +#include <linux/iopoll.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +#include <linux/spi/spi.h>
> +
> +/* Registers */
> +#define CSI_MODE               0x00    /* CSI mode control */
> +#define CSI_CLKSEL             0x04    /* CSI clock select */
> +#define CSI_CNT                        0x08    /* CSI control */
> +#define CSI_INT                        0x0C    /* CSI interrupt status */
> +#define CSI_IFIFOL             0x10    /* CSI receive FIFO level display */
> +#define CSI_OFIFOL             0x14    /* CSI transmit FIFO level display */
> +#define CSI_IFIFO              0x18    /* CSI receive window */
> +#define CSI_OFIFO              0x1C    /* CSI transmit window */
> +#define CSI_FIFOTRG            0x20    /* CSI FIFO trigger level */
> +
> +/* CSI_MODE */
> +#define CSI_MODE_CSIE          BIT(7)
> +#define CSI_MODE_TRMD          BIT(6)
> +#define CSI_MODE_CCL           BIT(5)
> +#define CSI_MODE_DIR           BIT(4)
> +#define CSI_MODE_CSOT          BIT(0)
> +
> +#define CSI_MODE_SETUP         0x00000040
> +
> +/* CSI_CLKSEL */
> +#define CSI_CLKSEL_CKP         BIT(17)
> +#define CSI_CLKSEL_DAP         BIT(16)
> +#define CSI_CLKSEL_SLAVE       BIT(15)
> +#define CSI_CLKSEL_CKS         GENMASK(14, 1)
> +
> +/* CSI_CNT */
> +#define CSI_CNT_CSIRST         BIT(28)
> +#define CSI_CNT_R_TRGEN                BIT(19)
> +#define CSI_CNT_UNDER_E                BIT(13)
> +#define CSI_CNT_OVERF_E                BIT(12)
> +#define CSI_CNT_TREND_E                BIT(9)
> +#define CSI_CNT_CSIEND_E       BIT(8)
> +#define CSI_CNT_T_TRGR_E       BIT(4)
> +#define CSI_CNT_R_TRGR_E       BIT(0)
> +
> +/* CSI_INT */
> +#define CSI_INT_UNDER          BIT(13)
> +#define CSI_INT_OVERF          BIT(12)
> +#define CSI_INT_TREND          BIT(9)
> +#define CSI_INT_CSIEND         BIT(8)
> +#define CSI_INT_T_TRGR         BIT(4)
> +#define CSI_INT_R_TRGR         BIT(0)
> +
> +/* CSI_FIFOTRG */
> +#define CSI_FIFOTRG_R_TRG       GENMASK(2, 0)
> +
> +#define CSI_FIFO_SIZE_BYTES    32
> +#define CSI_FIFO_HALF_SIZE     16
> +#define CSI_EN_DIS_TIMEOUT_US  100
> +#define CSI_CKS_MAX            0x3FFF
> +
> +#define UNDERRUN_ERROR         BIT(0)
> +#define OVERFLOW_ERROR         BIT(1)
> +#define TX_TIMEOUT_ERROR       BIT(2)
> +#define RX_TIMEOUT_ERROR       BIT(3)
> +
> +#define CSI_MAX_SPI_SCKO       8000000
> +
> +struct rzv2m_csi_priv {
> +       void __iomem *base;
> +       struct clk *csiclk;
> +       struct clk *pclk;
> +       struct device *dev;
> +       struct spi_controller *controller;
> +       const u8 *txbuf;
> +       u8 *rxbuf;
> +       int buffer_len;
> +       int bytes_sent;
> +       int bytes_received;
> +       int bytes_to_transfer;
> +       int words_to_transfer;

All these ints should be unsigned.

> +       unsigned char bytes_per_word;

3-byte gap

> +       wait_queue_head_t wait;
> +       u8 errors;

3 byte gap

> +       u32 status;
> +};

> +
> +static int rzv2m_csi_fill_txfifo(struct rzv2m_csi_priv *csi)
> +{
> +       int i;

unsigned int

> +
> +       if (readl(csi->base + CSI_OFIFOL))
> +               return -EIO;
> +
> +       if (csi->bytes_per_word == 2) {
> +               u16 *buf = (u16 *)csi->txbuf;

I think you can get rid of the casts by making rxbuf a const void *.

> +
> +               for (i = 0; i < csi->words_to_transfer; i++)
> +                       writel(buf[i], csi->base + CSI_OFIFO);
> +       } else {
> +               u8 *buf = (u8 *)csi->txbuf;
> +
> +               for (i = 0; i < csi->words_to_transfer; i++)
> +                       writel(buf[i], csi->base + CSI_OFIFO);
> +       }
> +
> +       csi->txbuf += csi->bytes_to_transfer;
> +       csi->bytes_sent += csi->bytes_to_transfer;
> +
> +       return 0;
> +}
> +
> +static int rzv2m_csi_read_rxfifo(struct rzv2m_csi_priv *csi)
> +{
> +       int i;
> +
> +       if (readl(csi->base + CSI_IFIFOL) != csi->bytes_to_transfer)
> +               return -EIO;
> +
> +       if (csi->bytes_per_word == 2) {
> +               u16 *buf = (u16 *)csi->rxbuf;

Similar for rxbuf.

> +
> +               for (i = 0; i < csi->words_to_transfer; i++)
> +                       buf[i] = (u16)readl(csi->base + CSI_IFIFO);
> +       } else {
> +               u8 *buf = (u8 *)csi->rxbuf;
> +
> +               for (i = 0; i < csi->words_to_transfer; i++)
> +                       buf[i] = (u8)readl(csi->base + CSI_IFIFO);
> +       }
> +
> +       csi->rxbuf += csi->bytes_to_transfer;
> +       csi->bytes_received += csi->bytes_to_transfer;
> +
> +       return 0;
> +}
> +
> +static inline void rzv2m_csi_calc_current_transfer(struct rzv2m_csi_priv *csi)
> +{
> +       int bytes_transferred = max_t(int, csi->bytes_received, csi->bytes_sent);
> +       int bytes_remaining = csi->buffer_len - bytes_transferred;
> +       int to_transfer;

unsigned...

> +
> +       if (csi->txbuf)
> +               /*
> +                * Leaving a little bit of headroom in the FIFOs makes it very
> +                * hard to raise an overflow error (which is only possible
> +                * when IP transmits and receives at the same time).
> +                */
> +               to_transfer = min_t(int, CSI_FIFO_HALF_SIZE, bytes_remaining);
> +       else
> +               to_transfer = min_t(int, CSI_FIFO_SIZE_BYTES, bytes_remaining);

Why min_t(int, ...)? Both values are int.

It would be better to make both unsigned, though.

> +
> +       if (csi->bytes_per_word == 2)
> +               to_transfer >>= 1;
> +
> +       /*
> +        * We can only choose a trigger level from a predefined set of values.
> +        * This will pick a value that is the greatest possible integer that's
> +        * less than or equal to the number of bytes we need to transfer.
> +        * This may result in multiple smaller transfers.
> +        */
> +       csi->words_to_transfer = x_trg_words[to_transfer - 1];
> +
> +       if (csi->bytes_per_word == 2)
> +               csi->bytes_to_transfer = csi->words_to_transfer << 1;
> +       else
> +               csi->bytes_to_transfer = csi->words_to_transfer;
> +}

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH v2 4/5] arm64: dts: renesas: r9a09g011: Add CSI nodes

[Geert Uytterhoeven]

On Thu, Jun 22, 2023 at 1:34 PM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
> The Renesas RZ/V2M comes with 6 Clocked Serial Interface (CSI)
> IPs (CSI0, CSI1, CSI2, CSI3, CSI4, CSI5), but Linux is only
> allowed access to CSI0 and CSI4.
>
> This commit adds SoC specific device tree support for CSI0 and
> CSI4.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> ---
>
> v2: no changes

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-devel for v6.6.

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH v2 5/5] arm64: defconfig: Enable Renesas RZ/V2M CSI driver

[Geert Uytterhoeven]

On Thu, Jun 22, 2023 at 1:34 PM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
> Enable CSI driver support for Renesas RZ/V2M based platforms.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> ---
>
> v2: no changes

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-devel for v6.6.

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH v2 5/5] arm64: defconfig: Enable Renesas RZ/V2M CSI driver

[Geert Uytterhoeven]

On Thu, Jun 22, 2023 at 1:34 PM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
> Enable CSI driver support for Renesas RZ/V2M based platforms.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> ---
>
> v2: no changes

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-devel for v6.6.

Gr{oetje,eeting}s,

                        Geert

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

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/5] spi: Add support for Renesas CSI

[andy.shevchenko]

Mon, Jul 03, 2023 at 12:19:26PM +0200, Geert Uytterhoeven kirjoitti:
> On Thu, Jun 22, 2023 at 1:34 PM Fabrizio Castro
> <fabrizio.castro.jz@renesas.com> wrote:

...

> > +       if (csi->txbuf)
> > +               /*
> > +                * Leaving a little bit of headroom in the FIFOs makes it very
> > +                * hard to raise an overflow error (which is only possible
> > +                * when IP transmits and receives at the same time).
> > +                */
> > +               to_transfer = min_t(int, CSI_FIFO_HALF_SIZE, bytes_remaining);
> > +       else
> > +               to_transfer = min_t(int, CSI_FIFO_SIZE_BYTES, bytes_remaining);
> 
> Why min_t(int, ...)? Both values are int.

min_t() should be used with a great care.

> It would be better to make both unsigned, though.

I believe you are assuming 3 (three) values and not 2 (two) under "both"
(one variable and two definitions).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/5] spi: Add support for Renesas CSI

[andy.shevchenko]

Thu, Jun 22, 2023 at 12:33:39PM +0100, Fabrizio Castro kirjoitti:
> The RZ/V2M SoC comes with the Clocked Serial Interface (CSI)
> IP, which is a master/slave SPI controller.
> 
> This commit adds a driver to support CSI master mode.

Submitting Patches recommends to use imperative voice.

...

+ bits.h

> +#include <linux/clk.h>
> +#include <linux/count_zeros.h>
> +#include <linux/interrupt.h>
> +#include <linux/iopoll.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +#include <linux/spi/spi.h>

...

> +#define CSI_CKS_MAX		0x3FFF

If it's limited by number of bits, i would explicitly use that information as
(BIT(14) - 1).

...

> +#define CSI_MAX_SPI_SCKO	8000000

Units?
Also, HZ_PER_MHZ?

...

> +static const unsigned char x_trg[] = {
> +	0, 1, 1, 2, 2, 2, 2, 3,
> +	3, 3, 3, 3, 3, 3, 3, 4,
> +	4, 4, 4, 4, 4, 4, 4, 4,
> +	4, 4, 4, 4, 4, 4, 4, 5
> +};
> +
> +static const unsigned char x_trg_words[] = {
> +	1,  2,  2,  4,  4,  4,  4,  8,
> +	8,  8,  8,  8,  8,  8,  8,  16,
> +	16, 16, 16, 16, 16, 16, 16, 16,
> +	16, 16, 16, 16, 16, 16, 16, 32
> +};

Why do you need tables? At the first glance these values can be easily
calculated from indexes.

...

> +	rzv2m_csi_reg_write_bit(csi, CSI_CNT, CSI_CNT_CSIRST, assert);
> +
> +	if (assert) {

	If (!assert)
		return 0;

> +		return readl_poll_timeout(csi->base + CSI_MODE, reg,
> +					  !(reg & CSI_MODE_CSOT), 0,
> +					  CSI_EN_DIS_TIMEOUT_US);
> +	}
> +
> +	return 0;

...

> +	rzv2m_csi_reg_write_bit(csi, CSI_MODE, CSI_MODE_CSIE, enable);
> +
> +	if (!enable && wait)

In the similar way.

> +		return readl_poll_timeout(csi->base + CSI_MODE, reg,
> +					  !(reg & CSI_MODE_CSOT), 0,
> +					  CSI_EN_DIS_TIMEOUT_US);
> +
> +	return 0;

...

> +		for (i = 0; i < csi->words_to_transfer; i++)
> +			writel(buf[i], csi->base + CSI_OFIFO);

writesl()?

...

> +		for (i = 0; i < csi->words_to_transfer; i++)
> +			buf[i] = (u16)readl(csi->base + CSI_IFIFO);

readsl()?

...

> +		for (i = 0; i < csi->words_to_transfer; i++)
> +			buf[i] = (u8)readl(csi->base + CSI_IFIFO);

readsl()?

...

Yes, in read cases you would need carefully handle the buffer data.
Or drop the idea if it looks too monsterous.

...

> +	ret = rzv2m_csi_wait_for_interrupt(csi, CSI_INT_TREND, CSI_CNT_TREND_E);

> +

Unneeded blank line.

> +	if (ret == -ETIMEDOUT)
> +		csi->errors |= TX_TIMEOUT_ERROR;

...

> +	struct rzv2m_csi_priv *csi = (struct rzv2m_csi_priv *)data;

From/to void * does not need an explicit casting in C.

...

> +	/* Setup clock polarity and phase timing */
> +	rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_CKP,
> +				!(spi->mode & SPI_CPOL));
> +	rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_DAP,
> +				!(spi->mode & SPI_CPHA));

Is it a must to do in a sequential writes?

...

> +	bool tx_completed = csi->txbuf ? false : true;
> +	bool rx_completed = csi->rxbuf ? false : true;

Ternaries are redundant in the above.

...

> +	controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST;

SPI_MODE_X_MASK

...

> +	controller->dev.of_node = pdev->dev.of_node;

device_set_node();

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/5] spi: Add support for Renesas CSI

[Geert Uytterhoeven]

Hi Andy,

On Wed, Jul 5, 2023 at 12:24 PM <andy.shevchenko@gmail.com> wrote:
> Mon, Jul 03, 2023 at 12:19:26PM +0200, Geert Uytterhoeven kirjoitti:
> > On Thu, Jun 22, 2023 at 1:34 PM Fabrizio Castro
> > <fabrizio.castro.jz@renesas.com> wrote:
> > > +       if (csi->txbuf)
> > > +               /*
> > > +                * Leaving a little bit of headroom in the FIFOs makes it very
> > > +                * hard to raise an overflow error (which is only possible
> > > +                * when IP transmits and receives at the same time).
> > > +                */
> > > +               to_transfer = min_t(int, CSI_FIFO_HALF_SIZE, bytes_remaining);
> > > +       else
> > > +               to_transfer = min_t(int, CSI_FIFO_SIZE_BYTES, bytes_remaining);
> >
> > Why min_t(int, ...)? Both values are int.
>
> min_t() should be used with a great care.
>
> > It would be better to make both unsigned, though.
>
> I believe you are assuming 3 (three) values and not 2 (two) under "both"
> (one variable and two definitions).

:-)

I meant "both numerical parametric values of each minimum operation".

Gr{oetje,eeting}s,

                        Geert

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

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

RE: [PATCH v2 3/5] spi: Add support for Renesas CSI

[Fabrizio Castro]

Hi Geert,

Thanks for your feedback!

> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Subject: Re: [PATCH v2 3/5] spi: Add support for Renesas CSI
> 
> Hi Fabrizio,
> 
> On Thu, Jun 22, 2023 at 1:34 PM Fabrizio Castro
> <fabrizio.castro.jz@renesas.com> wrote:
> > The RZ/V2M SoC comes with the Clocked Serial Interface (CSI)
> > IP, which is a master/slave SPI controller.
> >
> > This commit adds a driver to support CSI master mode.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > ---
> >
> > v2: edited includes in drivers/spi/spi-rzv2m-csi.c
> 
> Thanks for your patch!
> 
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -825,6 +825,12 @@ config SPI_RSPI
> >         help
> >           SPI driver for Renesas RSPI and QSPI blocks.
> >
> > +config SPI_RZV2M_CSI
> > +       tristate "Renesas RZV2M CSI controller"
> 
> RZ/V2M (patch sent)

Thanks for this.

> 
> > +       depends on ARCH_RENESAS || COMPILE_TEST
> > +       help
> > +         SPI driver for Renesas RZ/V2M Clocked Serial Interface (CSI)
> > +
> >  config SPI_QCOM_QSPI
> >         tristate "QTI QSPI controller"
> >         depends on ARCH_QCOM || COMPILE_TEST
> 
> > --- /dev/null
> > +++ b/drivers/spi/spi-rzv2m-csi.c
> > @@ -0,0 +1,667 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Renesas RZ/V2M Clocked Serial Interface (CSI) driver
> > + *
> > + * Copyright (C) 2023 Renesas Electronics Corporation
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/count_zeros.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reset.h>
> > +#include <linux/spi/spi.h>
> > +
> > +/* Registers */
> > +#define CSI_MODE               0x00    /* CSI mode control */
> > +#define CSI_CLKSEL             0x04    /* CSI clock select */
> > +#define CSI_CNT                        0x08    /* CSI control */
> > +#define CSI_INT                        0x0C    /* CSI interrupt status */
> > +#define CSI_IFIFOL             0x10    /* CSI receive FIFO level display
> */
> > +#define CSI_OFIFOL             0x14    /* CSI transmit FIFO level display
> */
> > +#define CSI_IFIFO              0x18    /* CSI receive window */
> > +#define CSI_OFIFO              0x1C    /* CSI transmit window */
> > +#define CSI_FIFOTRG            0x20    /* CSI FIFO trigger level */
> > +
> > +/* CSI_MODE */
> > +#define CSI_MODE_CSIE          BIT(7)
> > +#define CSI_MODE_TRMD          BIT(6)
> > +#define CSI_MODE_CCL           BIT(5)
> > +#define CSI_MODE_DIR           BIT(4)
> > +#define CSI_MODE_CSOT          BIT(0)
> > +
> > +#define CSI_MODE_SETUP         0x00000040
> > +
> > +/* CSI_CLKSEL */
> > +#define CSI_CLKSEL_CKP         BIT(17)
> > +#define CSI_CLKSEL_DAP         BIT(16)
> > +#define CSI_CLKSEL_SLAVE       BIT(15)
> > +#define CSI_CLKSEL_CKS         GENMASK(14, 1)
> > +
> > +/* CSI_CNT */
> > +#define CSI_CNT_CSIRST         BIT(28)
> > +#define CSI_CNT_R_TRGEN                BIT(19)
> > +#define CSI_CNT_UNDER_E                BIT(13)
> > +#define CSI_CNT_OVERF_E                BIT(12)
> > +#define CSI_CNT_TREND_E                BIT(9)
> > +#define CSI_CNT_CSIEND_E       BIT(8)
> > +#define CSI_CNT_T_TRGR_E       BIT(4)
> > +#define CSI_CNT_R_TRGR_E       BIT(0)
> > +
> > +/* CSI_INT */
> > +#define CSI_INT_UNDER          BIT(13)
> > +#define CSI_INT_OVERF          BIT(12)
> > +#define CSI_INT_TREND          BIT(9)
> > +#define CSI_INT_CSIEND         BIT(8)
> > +#define CSI_INT_T_TRGR         BIT(4)
> > +#define CSI_INT_R_TRGR         BIT(0)
> > +
> > +/* CSI_FIFOTRG */
> > +#define CSI_FIFOTRG_R_TRG       GENMASK(2, 0)
> > +
> > +#define CSI_FIFO_SIZE_BYTES    32
> > +#define CSI_FIFO_HALF_SIZE     16
> > +#define CSI_EN_DIS_TIMEOUT_US  100
> > +#define CSI_CKS_MAX            0x3FFF
> > +
> > +#define UNDERRUN_ERROR         BIT(0)
> > +#define OVERFLOW_ERROR         BIT(1)
> > +#define TX_TIMEOUT_ERROR       BIT(2)
> > +#define RX_TIMEOUT_ERROR       BIT(3)
> > +
> > +#define CSI_MAX_SPI_SCKO       8000000
> > +
> > +struct rzv2m_csi_priv {
> > +       void __iomem *base;
> > +       struct clk *csiclk;
> > +       struct clk *pclk;
> > +       struct device *dev;
> > +       struct spi_controller *controller;
> > +       const u8 *txbuf;
> > +       u8 *rxbuf;
> > +       int buffer_len;
> > +       int bytes_sent;
> > +       int bytes_received;
> > +       int bytes_to_transfer;
> > +       int words_to_transfer;
> 
> All these ints should be unsigned.

Will change.

> 
> > +       unsigned char bytes_per_word;
> 
> 3-byte gap
> 
> > +       wait_queue_head_t wait;
> > +       u8 errors;
> 
> 3 byte gap
> 
> > +       u32 status;
> > +};

I'll go over the gaps and improve.

> 
> > +
> > +static int rzv2m_csi_fill_txfifo(struct rzv2m_csi_priv *csi)
> > +{
> > +       int i;
> 
> unsigned int

Will change.

> 
> > +
> > +       if (readl(csi->base + CSI_OFIFOL))
> > +               return -EIO;
> > +
> > +       if (csi->bytes_per_word == 2) {
> > +               u16 *buf = (u16 *)csi->txbuf;
> 
> I think you can get rid of the casts by making rxbuf a const void *.

I'll try and see if I can improve this.

> 
> > +
> > +               for (i = 0; i < csi->words_to_transfer; i++)
> > +                       writel(buf[i], csi->base + CSI_OFIFO);
> > +       } else {
> > +               u8 *buf = (u8 *)csi->txbuf;
> > +
> > +               for (i = 0; i < csi->words_to_transfer; i++)
> > +                       writel(buf[i], csi->base + CSI_OFIFO);
> > +       }
> > +
> > +       csi->txbuf += csi->bytes_to_transfer;
> > +       csi->bytes_sent += csi->bytes_to_transfer;
> > +
> > +       return 0;
> > +}
> > +
> > +static int rzv2m_csi_read_rxfifo(struct rzv2m_csi_priv *csi)
> > +{
> > +       int i;
> > +
> > +       if (readl(csi->base + CSI_IFIFOL) != csi->bytes_to_transfer)
> > +               return -EIO;
> > +
> > +       if (csi->bytes_per_word == 2) {
> > +               u16 *buf = (u16 *)csi->rxbuf;
> 
> Similar for rxbuf.

Okay.

> 
> > +
> > +               for (i = 0; i < csi->words_to_transfer; i++)
> > +                       buf[i] = (u16)readl(csi->base + CSI_IFIFO);
> > +       } else {
> > +               u8 *buf = (u8 *)csi->rxbuf;
> > +
> > +               for (i = 0; i < csi->words_to_transfer; i++)
> > +                       buf[i] = (u8)readl(csi->base + CSI_IFIFO);
> > +       }
> > +
> > +       csi->rxbuf += csi->bytes_to_transfer;
> > +       csi->bytes_received += csi->bytes_to_transfer;
> > +
> > +       return 0;
> > +}
> > +
> > +static inline void rzv2m_csi_calc_current_transfer(struct rzv2m_csi_priv
> *csi)
> > +{
> > +       int bytes_transferred = max_t(int, csi->bytes_received, csi-
> >bytes_sent);
> > +       int bytes_remaining = csi->buffer_len - bytes_transferred;
> > +       int to_transfer;
> 
> unsigned...

Okay

> 
> > +
> > +       if (csi->txbuf)
> > +               /*
> > +                * Leaving a little bit of headroom in the FIFOs makes it
> very
> > +                * hard to raise an overflow error (which is only possible
> > +                * when IP transmits and receives at the same time).
> > +                */
> > +               to_transfer = min_t(int, CSI_FIFO_HALF_SIZE,
> bytes_remaining);
> > +       else
> > +               to_transfer = min_t(int, CSI_FIFO_SIZE_BYTES,
> bytes_remaining);
> 
> Why min_t(int, ...)? Both values are int.
> 
> It would be better to make both unsigned, though.

Will do.

> 
> > +
> > +       if (csi->bytes_per_word == 2)
> > +               to_transfer >>= 1;
> > +
> > +       /*
> > +        * We can only choose a trigger level from a predefined set of
> values.
> > +        * This will pick a value that is the greatest possible integer
> that's
> > +        * less than or equal to the number of bytes we need to transfer.
> > +        * This may result in multiple smaller transfers.
> > +        */
> > +       csi->words_to_transfer = x_trg_words[to_transfer - 1];
> > +
> > +       if (csi->bytes_per_word == 2)
> > +               csi->bytes_to_transfer = csi->words_to_transfer << 1;
> > +       else
> > +               csi->bytes_to_transfer = csi->words_to_transfer;
> > +}

Thanks,
Fab

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
>                                 -- Linus Torvalds

RE: [PATCH v2 3/5] spi: Add support for Renesas CSI

[Fabrizio Castro]

Hi Andy,

Thanks for your feedback!

> From: andy.shevchenko@gmail.com <andy.shevchenko@gmail.com>
> Subject: Re: [PATCH v2 3/5] spi: Add support for Renesas CSI
> 
> Thu, Jun 22, 2023 at 12:33:39PM +0100, Fabrizio Castro kirjoitti:
> > The RZ/V2M SoC comes with the Clocked Serial Interface (CSI)
> > IP, which is a master/slave SPI controller.
> >
> > This commit adds a driver to support CSI master mode.
> 
> Submitting Patches recommends to use imperative voice.
> 
> ...
> 
> + bits.h

Okay

> 
> > +#include <linux/clk.h>
> > +#include <linux/count_zeros.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reset.h>
> > +#include <linux/spi/spi.h>
> 
> ...
> 
> > +#define CSI_CKS_MAX		0x3FFF
> 
> If it's limited by number of bits, i would explicitly use that information
> as
> (BIT(14) - 1).

That value represents the register setting for the maximum clock divider.
The maximum divider and corresponding register setting are plainly stated
in the HW User Manual, therefore I would like to use either (plain) value
to make it easier for the reader.

I think perhaps the below makes this clearer:
#define CSI_CKS_MAX_DIV_RATIO   32766
#define CSI_CKS_MAX             (CSI_CKS_MAX_DIV_RATIO >> 1)


> 
> ...
> 
> > +#define CSI_MAX_SPI_SCKO	8000000
> 
> Units?
> Also, HZ_PER_MHZ?

I'll replace that with:
#define CSI_MAX_SPI_SCKO        (8 * HZ_PER_MHZ)

> 
> ...
> 
> > +static const unsigned char x_trg[] = {
> > +	0, 1, 1, 2, 2, 2, 2, 3,
> > +	3, 3, 3, 3, 3, 3, 3, 4,
> > +	4, 4, 4, 4, 4, 4, 4, 4,
> > +	4, 4, 4, 4, 4, 4, 4, 5
> > +};
> > +
> > +static const unsigned char x_trg_words[] = {
> > +	1,  2,  2,  4,  4,  4,  4,  8,
> > +	8,  8,  8,  8,  8,  8,  8,  16,
> > +	16, 16, 16, 16, 16, 16, 16, 16,
> > +	16, 16, 16, 16, 16, 16, 16, 32
> > +};
> 
> Why do you need tables? At the first glance these values can be easily
> calculated from indexes.

I think I am going to replace those tables with the below (and of course
adjust the callers accordingly since the argument is not an index anymore):

static inline unsigned int x_trg(unsigned int words)
{
	return fls(words) - 1;
}

static inline unsigned int x_trg_words(unsigned int words)
{
	return 1 << x_trg(words);
}

> 
> ...
> 
> > +	rzv2m_csi_reg_write_bit(csi, CSI_CNT, CSI_CNT_CSIRST, assert);
> > +
> > +	if (assert) {
> 
> 	If (!assert)
> 		return 0;

Can do

> 
> > +		return readl_poll_timeout(csi->base + CSI_MODE, reg,
> > +					  !(reg & CSI_MODE_CSOT), 0,
> > +					  CSI_EN_DIS_TIMEOUT_US);
> > +	}
> > +
> > +	return 0;
> 
> ...
> 
> > +	rzv2m_csi_reg_write_bit(csi, CSI_MODE, CSI_MODE_CSIE, enable);
> > +
> > +	if (!enable && wait)
> 
> In the similar way.

Okay

> 
> > +		return readl_poll_timeout(csi->base + CSI_MODE, reg,
> > +					  !(reg & CSI_MODE_CSOT), 0,
> > +					  CSI_EN_DIS_TIMEOUT_US);
> > +
> > +	return 0;
> 
> ...
> 
> > +		for (i = 0; i < csi->words_to_transfer; i++)
> > +			writel(buf[i], csi->base + CSI_OFIFO);
> 
> writesl()?

I think you mean writesw and writesb.
They should simplify things a bit, I'll go for them.

> 
> ...
> 
> > +		for (i = 0; i < csi->words_to_transfer; i++)
> > +			buf[i] = (u16)readl(csi->base + CSI_IFIFO);
> 
> readsl()?

I'll use readsw

> 
> ...
> 
> > +		for (i = 0; i < csi->words_to_transfer; i++)
> > +			buf[i] = (u8)readl(csi->base + CSI_IFIFO);
> 
> readsl()?

I'll use readsb

> 
> ...
> 
> Yes, in read cases you would need carefully handle the buffer data.
> Or drop the idea if it looks too monsterous.

It should be okay in my case. Thanks.

> 
> ...
> 
> > +	ret = rzv2m_csi_wait_for_interrupt(csi, CSI_INT_TREND,
> CSI_CNT_TREND_E);
> 
> > +
> 
> Unneeded blank line.

Will take out

> 
> > +	if (ret == -ETIMEDOUT)
> > +		csi->errors |= TX_TIMEOUT_ERROR;
> 
> ...
> 
> > +	struct rzv2m_csi_priv *csi = (struct rzv2m_csi_priv *)data;
> 
> From/to void * does not need an explicit casting in C.

Of course.

> 
> ...
> 
> > +	/* Setup clock polarity and phase timing */
> > +	rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_CKP,
> > +				!(spi->mode & SPI_CPOL));
> > +	rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_DAP,
> > +				!(spi->mode & SPI_CPHA));
> 
> Is it a must to do in a sequential writes?

It's not a must, I'll combine those 2 statements into 1.

> 
> ...
> 
> > +	bool tx_completed = csi->txbuf ? false : true;
> > +	bool rx_completed = csi->rxbuf ? false : true;
> 
> Ternaries are redundant in the above.

They are also horrible, the below should do:
bool tx_completed = !csi->txbuf;
bool rx_completed = !csi->rxbuf;

> 
> ...
> 
> > +	controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST;
> 
> SPI_MODE_X_MASK

This statement sets the mode_bits. Using a macro meant to be used as a
mask in this context is something I would want to avoid if possible.

> 
> ...
> 
> > +	controller->dev.of_node = pdev->dev.of_node;
> 
> device_set_node();

Will change.

I'll send an enhancement patch shortly to reflect your suggestions and
also Geert's.

Thanks for your help!

Cheers,
Fab

> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Re: [PATCH v2 3/5] spi: Add support for Renesas CSI

[Andy Shevchenko]

On Thu, Jul 13, 2023 at 6:52 PM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:

...

> > > +#define CSI_CKS_MAX                0x3FFF
> >
> > If it's limited by number of bits, i would explicitly use that information
> > as
> > (BIT(14) - 1).
>
> That value represents the register setting for the maximum clock divider.
> The maximum divider and corresponding register setting are plainly stated
> in the HW User Manual, therefore I would like to use either (plain) value
> to make it easier for the reader.
>
> I think perhaps the below makes this clearer:
> #define CSI_CKS_MAX_DIV_RATIO   32766

Hmm... To me it's a bit confusing now. Shouldn't it be 32767?

> #define CSI_CKS_MAX             (CSI_CKS_MAX_DIV_RATIO >> 1)

Whatever you choose it would be better to add a comment to explain
this. Because the above is more clear to me with BIT(14)-1 if the
register field is 14-bit long.
With this value(s) I'm lost. Definitely needs a comment.

...

> > > +static const unsigned char x_trg[] = {
> > > +   0, 1, 1, 2, 2, 2, 2, 3,
> > > +   3, 3, 3, 3, 3, 3, 3, 4,
> > > +   4, 4, 4, 4, 4, 4, 4, 4,
> > > +   4, 4, 4, 4, 4, 4, 4, 5
> > > +};
> > > +
> > > +static const unsigned char x_trg_words[] = {
> > > +   1,  2,  2,  4,  4,  4,  4,  8,
> > > +   8,  8,  8,  8,  8,  8,  8,  16,
> > > +   16, 16, 16, 16, 16, 16, 16, 16,
> > > +   16, 16, 16, 16, 16, 16, 16, 32
> > > +};
> >
> > Why do you need tables? At the first glance these values can be easily
> > calculated from indexes.
>
> I think I am going to replace those tables with the below (and of course
> adjust the callers accordingly since the argument is not an index anymore):
>
> static inline unsigned int x_trg(unsigned int words)
> {
>         return fls(words) - 1;
> }

OK, but I think you can use it just inplace, no need to have such as a
standalone function.

> static inline unsigned int x_trg_words(unsigned int words)
> {
>         return 1 << x_trg(words);
> }

Besides a better form of BIT(...) this looks to me like NIH
roundup_pow_of_two().

...

> > > +   /* Setup clock polarity and phase timing */
> > > +   rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_CKP,
> > > +                           !(spi->mode & SPI_CPOL));
> > > +   rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_DAP,
> > > +                           !(spi->mode & SPI_CPHA));
> >
> > Is it a must to do in a sequential writes?
>
> It's not a must, I'll combine those 2 statements into 1.

If so, you can use SPI_MODE_X_MASK.

...

> > > +   controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST;
> >
> > SPI_MODE_X_MASK
>
> This statement sets the mode_bits. Using a macro meant to be used as a
> mask in this context is something I would want to avoid if possible.

Hmm... not a big deal, but I think that's what covers all mode_bits,
and mode_bits by nature _is_ a mask.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v2 3/5] spi: Add support for Renesas CSI

[Fabrizio Castro]

Hi Andy,

Thanks for your reply!

> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Subject: Re: [PATCH v2 3/5] spi: Add support for Renesas CSI
> 
> On Thu, Jul 13, 2023 at 6:52 PM Fabrizio Castro
> <fabrizio.castro.jz@renesas.com> wrote:
> 
> ...
> 
> > > > +#define CSI_CKS_MAX                0x3FFF
> > >
> > > If it's limited by number of bits, i would explicitly use that
> information
> > > as
> > > (BIT(14) - 1).
> >
> > That value represents the register setting for the maximum clock
> divider.
> > The maximum divider and corresponding register setting are plainly
> stated
> > in the HW User Manual, therefore I would like to use either (plain)
> value
> > to make it easier for the reader.
> >
> > I think perhaps the below makes this clearer:
> > #define CSI_CKS_MAX_DIV_RATIO   32766
> 
> Hmm... To me it's a bit confusing now. Shouldn't it be 32767?

32766 is the correct value.

Clock "csiclk" gets divided by 2 * CSI_CLKSEL_CKS in order to generate the
serial clock (output from master), with CSI_CLKSEL_CKS ranging from 0x1 (that
means "csiclk" is divided by 2) to 0x3FFF ("csiclk" is divided by 32766).

> 
> > #define CSI_CKS_MAX             (CSI_CKS_MAX_DIV_RATIO >> 1)
> 
> Whatever you choose it would be better to add a comment to explain
> this. Because the above is more clear to me with BIT(14)-1 if the
> register field is 14-bit long.
> With this value(s) I'm lost. Definitely needs a comment.

To cater for a wider audience (and not just for those who have read the
HW manual), I think perhaps the below would probably be the best compromise:

/*
 * Clock "csiclk" gets divided by 2 * CSI_CLKSEL_CKS in order to generate the
 * serial clock (output from master), with CSI_CLKSEL_CKS ranging from 0x1 (that
 * means "csiclk" is divided by 2) to 0x3FFF ("csiclk" is divided by 32766).
 */
#define CSI_CKS_MAX             (BIT(14)-1)

> 
> ...
> 
> >
> > static inline unsigned int x_trg(unsigned int words)
> > {
> >         return fls(words) - 1;
> > }
> 
> OK, but I think you can use it just inplace, no need to have such as a
> standalone function.

The above is actually equivalent to ilog2()

> 
> > static inline unsigned int x_trg_words(unsigned int words)
> > {
> >         return 1 << x_trg(words);
> > }
> 
> Besides a better form of BIT(...) this looks to me like NIH
> roundup_pow_of_two().

rounddown_pow_of_two().

I have tested the driver with s/x_trg/ilog2 and
s/x_trg_words/roundup_pow_of_two and it looks like I am losing tiny bit of
performance (probably down to the use of ternary operators in both macros)
but I think it's okay, let's not reinvent the wheel and let's keep it more
readable, I'll switch to using the above macros.

> 
> ...
> 
> > > > +   /* Setup clock polarity and phase timing */
> > > > +   rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_CKP,
> > > > +                           !(spi->mode & SPI_CPOL));
> > > > +   rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_DAP,
> > > > +                           !(spi->mode & SPI_CPHA));
> > >
> > > Is it a must to do in a sequential writes?
> >
> > It's not a must, I'll combine those 2 statements into 1.
> 
> If so, you can use SPI_MODE_X_MASK.

That's the plan.

Thanks for your help Andy.

Cheers,
Fab

> 
> ...
> 
> > > > +   controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST;
> > >
> > > SPI_MODE_X_MASK
> >
> > This statement sets the mode_bits. Using a macro meant to be used as
> a
> > mask in this context is something I would want to avoid if possible.
> 
> Hmm... not a big deal, but I think that's what covers all mode_bits,
> and mode_bits by nature _is_ a mask.
> 
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v2 3/5] spi: Add support for Renesas CSI

[Geert Uytterhoeven]

Hi Fabrizio,

On Fri, Jul 14, 2023 at 12:35 AM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Subject: Re: [PATCH v2 3/5] spi: Add support for Renesas CSI
> >
> > On Thu, Jul 13, 2023 at 6:52 PM Fabrizio Castro
> > <fabrizio.castro.jz@renesas.com> wrote:
> >
> > ...
> >
> > > > > +#define CSI_CKS_MAX                0x3FFF
> > > >
> > > > If it's limited by number of bits, i would explicitly use that
> > information
> > > > as
> > > > (BIT(14) - 1).
> > >
> > > That value represents the register setting for the maximum clock
> > divider.
> > > The maximum divider and corresponding register setting are plainly
> > stated
> > > in the HW User Manual, therefore I would like to use either (plain)
> > value
> > > to make it easier for the reader.
> > >
> > > I think perhaps the below makes this clearer:
> > > #define CSI_CKS_MAX_DIV_RATIO   32766
> >
> > Hmm... To me it's a bit confusing now. Shouldn't it be 32767?
>
> 32766 is the correct value.
>
> Clock "csiclk" gets divided by 2 * CSI_CLKSEL_CKS in order to generate the
> serial clock (output from master), with CSI_CLKSEL_CKS ranging from 0x1 (that
> means "csiclk" is divided by 2) to 0x3FFF ("csiclk" is divided by 32766).
>
> >
> > > #define CSI_CKS_MAX             (CSI_CKS_MAX_DIV_RATIO >> 1)
> >
> > Whatever you choose it would be better to add a comment to explain
> > this. Because the above is more clear to me with BIT(14)-1 if the
> > register field is 14-bit long.
> > With this value(s) I'm lost. Definitely needs a comment.
>
> To cater for a wider audience (and not just for those who have read the
> HW manual), I think perhaps the below would probably be the best compromise:
>
> /*
>  * Clock "csiclk" gets divided by 2 * CSI_CLKSEL_CKS in order to generate the
>  * serial clock (output from master), with CSI_CLKSEL_CKS ranging from 0x1 (that
>  * means "csiclk" is divided by 2) to 0x3FFF ("csiclk" is divided by 32766).
>  */
> #define CSI_CKS_MAX             (BIT(14)-1)

Or GENMASK(13, 0)

As we have

    #define CSI_CLKSEL_CKS          GENMASK(14, 1)

and bit 0 must of the CLKSEL register must always be zero, the actual
divider is incidentally FIELD_GET(GENMASK(14, 0), clksel).
No idea if that can be useful to simplify the code, though ;-)

> > > static inline unsigned int x_trg(unsigned int words)
> > > {
> > >         return fls(words) - 1;
> > > }
> >
> > OK, but I think you can use it just inplace, no need to have such as a
> > standalone function.
>
> The above is actually equivalent to ilog2()
>
> >
> > > static inline unsigned int x_trg_words(unsigned int words)
> > > {
> > >         return 1 << x_trg(words);
> > > }
> >
> > Besides a better form of BIT(...) this looks to me like NIH
> > roundup_pow_of_two().
>
> rounddown_pow_of_two().
>
> I have tested the driver with s/x_trg/ilog2 and
> s/x_trg_words/roundup_pow_of_two and it looks like I am losing tiny bit of
> performance (probably down to the use of ternary operators in both macros)
> but I think it's okay, let's not reinvent the wheel and let's keep it more
> readable, I'll switch to using the above macros.

You mean this is not lost in the noise of the big loop in
rzv2m_csi_pio_transfer(), which is even waiting on an event?
I find that a bit surprising...

Gr{oetje,eeting}s,

                        Geert

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

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

RE: [PATCH v2 3/5] spi: Add support for Renesas CSI

[Fabrizio Castro]

Hi Geert,

Thanks your reply!

> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Subject: Re: [PATCH v2 3/5] spi: Add support for Renesas CSI
> 
> Hi Fabrizio,
> 
> On Fri, Jul 14, 2023 at 12:35 AM Fabrizio Castro
> <fabrizio.castro.jz@renesas.com> wrote:
> > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Subject: Re: [PATCH v2 3/5] spi: Add support for Renesas CSI
> > >
> > > On Thu, Jul 13, 2023 at 6:52 PM Fabrizio Castro
> > > <fabrizio.castro.jz@renesas.com> wrote:
> > >
> > > ...
> > >
> > > > > > +#define CSI_CKS_MAX                0x3FFF
> > > > >
> > > > > If it's limited by number of bits, i would explicitly use that
> > > information
> > > > > as
> > > > > (BIT(14) - 1).
> > > >
> > > > That value represents the register setting for the maximum clock
> > > divider.
> > > > The maximum divider and corresponding register setting are
> plainly
> > > stated
> > > > in the HW User Manual, therefore I would like to use either
> (plain)
> > > value
> > > > to make it easier for the reader.
> > > >
> > > > I think perhaps the below makes this clearer:
> > > > #define CSI_CKS_MAX_DIV_RATIO   32766
> > >
> > > Hmm... To me it's a bit confusing now. Shouldn't it be 32767?
> >
> > 32766 is the correct value.
> >
> > Clock "csiclk" gets divided by 2 * CSI_CLKSEL_CKS in order to
> generate the
> > serial clock (output from master), with CSI_CLKSEL_CKS ranging from
> 0x1 (that
> > means "csiclk" is divided by 2) to 0x3FFF ("csiclk" is divided by
> 32766).
> >
> > >
> > > > #define CSI_CKS_MAX             (CSI_CKS_MAX_DIV_RATIO >> 1)
> > >
> > > Whatever you choose it would be better to add a comment to explain
> > > this. Because the above is more clear to me with BIT(14)-1 if the
> > > register field is 14-bit long.
> > > With this value(s) I'm lost. Definitely needs a comment.
> >
> > To cater for a wider audience (and not just for those who have read
> the
> > HW manual), I think perhaps the below would probably be the best
> compromise:
> >
> > /*
> >  * Clock "csiclk" gets divided by 2 * CSI_CLKSEL_CKS in order to
> generate the
> >  * serial clock (output from master), with CSI_CLKSEL_CKS ranging
> from 0x1 (that
> >  * means "csiclk" is divided by 2) to 0x3FFF ("csiclk" is divided by
> 32766).
> >  */
> > #define CSI_CKS_MAX             (BIT(14)-1)
> 
> Or GENMASK(13, 0)

Yeah.

> 
> As we have
> 
>     #define CSI_CLKSEL_CKS          GENMASK(14, 1)
> 
> and bit 0 must of the CLKSEL register must always be zero, the actual
> divider is incidentally FIELD_GET(GENMASK(14, 0), clksel).
> No idea if that can be useful to simplify the code, though ;-)

Thanks for pointing this out. Will have a look, but no promises ;-)

> 
> > > > static inline unsigned int x_trg(unsigned int words)
> > > > {
> > > >         return fls(words) - 1;
> > > > }
> > >
> > > OK, but I think you can use it just inplace, no need to have such
> as a
> > > standalone function.
> >
> > The above is actually equivalent to ilog2()
> >
> > >
> > > > static inline unsigned int x_trg_words(unsigned int words)
> > > > {
> > > >         return 1 << x_trg(words);
> > > > }
> > >
> > > Besides a better form of BIT(...) this looks to me like NIH
> > > roundup_pow_of_two().
> >
> > rounddown_pow_of_two().
> >
> > I have tested the driver with s/x_trg/ilog2 and
> > s/x_trg_words/roundup_pow_of_two and it looks like I am losing tiny
> bit of
> > performance (probably down to the use of ternary operators in both
> macros)
> > but I think it's okay, let's not reinvent the wheel and let's keep
> it more
> > readable, I'll switch to using the above macros.
> 
> You mean this is not lost in the noise of the big loop in
> rzv2m_csi_pio_transfer(), which is even waiting on an event?
> I find that a bit surprising...

Those calculations get done when no TX/RX is in progress, and they are
executed for every burst (as they are used to decide how many bytes
in the FIFOs to use for the current burst), therefore they add a delay
to the whole thing.

It's only a tiny drop, about 0.4% .

Cheers,
Fab

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a
> hacker. But
> when I'm talking to journalists I just say "programmer" or something
> like that.
>                                 -- Linus Torvalds