Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
MIME-Version: 1.0
References: <20230926210818.197356-1-fabrizio.castro.jz@renesas.com> <20230926210818.197356-3-fabrizio.castro.jz@renesas.com>
In-Reply-To: <20230926210818.197356-3-fabrizio.castro.jz@renesas.com>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Wed, 27 Sep 2023 11:51:39 +0200
Message-ID: <CAMuHMdWUr5ReouCWV3WsOJE=2faBO-y70aDC8W8qBGVBhaMTAg@mail.gmail.com>
Subject: Re: [PATCH 2/2] spi: rzv2m-csi: Add Slave mode support
To:     Fabrizio Castro <fabrizio.castro.jz@renesas.com>
Cc:     Mark Brown <broonie@kernel.org>, Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        Magnus Damm <magnus.damm@gmail.com>, linux-spi@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-renesas-soc@vger.kernel.org,
        Chris Paterson <Chris.Paterson2@renesas.com>,
        Biju Das <biju.das@bp.renesas.com>,
        Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

Hi Fabrizio,

On Tue, Sep 26, 2023 at 11:08 PM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
> The CSI IP found inside the Renesas RZ/V2M SoC supports
> both SPI Master and SPI Slave roles.
>
> When working in slave mode, the CSI IP has the option
> of using its Slave Select (SS) pin to enable TX and RX
> operations. Since the SPI slave cannot control the clock,
> when working as slave it's best not to stop operations
> during a transfer, as by doing so the IP will not send or
> receive data, regardless of clock and active level on pin SS.
> A side effect from not stopping operations is that the RX
> FIFO needs to be flushed, word by word, when RX data needs
> to be discarded.
>
> Finally, when in slave mode timings are tighter, as missing a
> deadline translates to errors being thrown, resulting in
> aborting the transfer. In order to speed things up, we can
> avoid waiting for the TX FIFO to be empty, we can just wait
> for the RX FIFO to contain at least the number of words that
> we expect.
>
> Add slave support to the currently existing CSI driver.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>

Thanks for your patch!

> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -861,8 +861,10 @@ config SPI_RSPI
>  config SPI_RZV2M_CSI
>         tristate "Renesas RZ/V2M CSI controller"
>         depends on ARCH_RENESAS || COMPILE_TEST
> +       depends on SPI_SLAVE

Isn't that a bit too strict?
The driver can/should be used/usable in host mode when SPI_SLAVE
is not enabled.

>         help
> -         SPI driver for Renesas RZ/V2M Clocked Serial Interface (CSI)
> +         SPI driver for Renesas RZ/V2M Clocked Serial Interface (CSI).
> +         CSI supports master and slave roles.
>
>  config SPI_QCOM_QSPI
>         tristate "QTI QSPI controller"

> --- a/drivers/spi/spi-rzv2m-csi.c
> +++ b/drivers/spi/spi-rzv2m-csi.c

> @@ -99,6 +112,9 @@ struct rzv2m_csi_priv {
>         wait_queue_head_t wait;
>         u32 errors;
>         u32 status;
> +       int mode;

Do you need this flag?
You can use spi_controller_is_target() instead.

> +       int slave_select;
> +       bool slave_aborted;
>  };
>
>  static void rzv2m_csi_reg_write_bit(const struct rzv2m_csi_priv *csi,

> @@ -279,32 +303,23 @@ static int rzv2m_csi_wait_for_interrupt(struct rzv2m_csi_priv *csi,
>
>         rzv2m_csi_enable_irqs(csi, enable_bits);
>
> -       ret = wait_event_timeout(csi->wait,
> -                                ((csi->status & wait_mask) == wait_mask) ||
> -                                csi->errors, HZ);
> +       if (csi->mode == RZV2M_CSI_SPI_SLAVE) {

spi_controller_is_target()

> +               ret = wait_event_interruptible(csi->wait,
> +                               ((csi->status & wait_mask) == wait_mask) ||
> +                               csi->errors || csi->slave_aborted);

target_aborted (everywhere)

> +               if (ret || csi->slave_aborted)
> +                       ret = -EINTR;
> +       } else {
> +               ret = wait_event_timeout(csi->wait,
> +                               ((csi->status & wait_mask) == wait_mask) ||
> +                               csi->errors, HZ) == 0 ? -ETIMEDOUT : 0;
> +       }

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