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

[Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx>]

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