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

From: Geert Uytterhoeven
Date: Mon Jul 03 2023 - 06:19:58 EST

Next message: Sean Nyekjaer: "Re: [PATCH] i2c: stm32f7: Add atomic_xfer method to driver"
Previous message: Lee, Kah Jing: "RE: [v4] arm64: dts: agilex/stratix10: Updated QSPI Flash layout for UBIFS"
Next in thread: andy . shevchenko: "Re: [PATCH v2 3/5] spi: Add support for Renesas CSI"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Fabrizio,

On Thu, Jun 22, 2023 at 1:34 PM Fabrizio Castro
<fabrizio.castro.jz@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> ---
>
> 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@xxxxxxxxxxxxxx

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

Next message: Sean Nyekjaer: "Re: [PATCH] i2c: stm32f7: Add atomic_xfer method to driver"
Previous message: Lee, Kah Jing: "RE: [v4] arm64: dts: agilex/stratix10: Updated QSPI Flash layout for UBIFS"
Next in thread: andy . shevchenko: "Re: [PATCH v2 3/5] spi: Add support for Renesas CSI"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]