Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230622113341.657842-1-fabrizio.castro.jz@renesas.com> <20230622113341.657842-4-fabrizio.castro.jz@renesas.com>
In-Reply-To: <20230622113341.657842-4-fabrizio.castro.jz@renesas.com>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Mon, 3 Jul 2023 12:19:26 +0200
Message-ID: <CAMuHMdVsYohH5FVv6r4ha0AaHRoHjNF1ErjW1FNF7ZAQR9ntxg@mail.gmail.com>
Subject: Re: [PATCH v2 3/5] spi: Add support for Renesas CSI
To:     Fabrizio Castro <fabrizio.castro.jz@renesas.com>
Cc:     Mark Brown <broonie@kernel.org>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Magnus Damm <magnus.damm@gmail.com>,
        linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org,
        linux-renesas-soc@vger.kernel.org,
        Chris Paterson <Chris.Paterson2@renesas.com>,
        Biju Das <biju.das@bp.renesas.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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