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
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 <[email protected]>
---
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 <[email protected]>");
+MODULE_DESCRIPTION("Clocked Serial Interface Driver");
--
2.34.1
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 <[email protected]>
---
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
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
Hi Mark,
On Fri, Jun 23, 2023 at 2:32 AM Mark Brown <[email protected]> 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 -- [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 Fri, Jun 23, 2023 at 08:49:05AM +0200, Geert Uytterhoeven wrote:
> On Fri, Jun 23, 2023 at 2:32 AM Mark Brown <[email protected]> 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.
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
Hi Fabrizio,
On Thu, Jun 22, 2023 at 1:34 PM Fabrizio Castro
<[email protected]> 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 <[email protected]>
> ---
>
> 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 -- [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 Thu, Jun 22, 2023 at 1:34 PM Fabrizio Castro
<[email protected]> 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 <[email protected]>
> ---
>
> v2: no changes
Reviewed-by: Geert Uytterhoeven <[email protected]>
i.e. will queue in renesas-devel for v6.6.
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
Mon, Jul 03, 2023 at 12:19:26PM +0200, Geert Uytterhoeven kirjoitti:
> On Thu, Jun 22, 2023 at 1:34 PM Fabrizio Castro
> <[email protected]> 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
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
Hi Andy,
On Wed, Jul 5, 2023 at 12:24 PM <[email protected]> wrote:
> Mon, Jul 03, 2023 at 12:19:26PM +0200, Geert Uytterhoeven kirjoitti:
> > On Thu, Jun 22, 2023 at 1:34 PM Fabrizio Castro
> > <[email protected]> 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 -- [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 Geert,
Thanks for your feedback!
> From: Geert Uytterhoeven <[email protected]>
> 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
> <[email protected]> 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 <[email protected]>
> > ---
> >
> > 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
Hi Andy,
Thanks for your feedback!
> From: [email protected] <[email protected]>
> 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
>
On Thu, Jul 13, 2023 at 6:52 PM Fabrizio Castro
<[email protected]> 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
Hi Andy,
Thanks for your reply!
> From: Andy Shevchenko <[email protected]>
> Subject: Re: [PATCH v2 3/5] spi: Add support for Renesas CSI
>
> On Thu, Jul 13, 2023 at 6:52 PM Fabrizio Castro
> <[email protected]> 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
Hi Fabrizio,
On Fri, Jul 14, 2023 at 12:35 AM Fabrizio Castro
<[email protected]> wrote:
> > From: Andy Shevchenko <[email protected]>
> > Subject: Re: [PATCH v2 3/5] spi: Add support for Renesas CSI
> >
> > On Thu, Jul 13, 2023 at 6:52 PM Fabrizio Castro
> > <[email protected]> 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 -- [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 Geert,
Thanks your reply!
> From: Geert Uytterhoeven <[email protected]>
> 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
> <[email protected]> wrote:
> > > From: Andy Shevchenko <[email protected]>
> > > Subject: Re: [PATCH v2 3/5] spi: Add support for Renesas CSI
> > >
> > > On Thu, Jul 13, 2023 at 6:52 PM Fabrizio Castro
> > > <[email protected]> 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 --
> [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