Re: [PATCH 2/2] spi: rzv2m-csi: Add Slave mode support

[Geert Uytterhoeven]

Hi Fabrizio,

On Tue, Sep 26, 2023 at 11:08 PM Fabrizio Castro
<fabrizio.castro.jz@xxxxxxxxxxx> 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@xxxxxxxxxxx>

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@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