Hi Geert,
Thanks for your feedback!
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> 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@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)
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
