Hi Geert,
Thanks for your reply!
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Subject: Re: [PATCH 2/2] spi: rzv2m-csi: Add Slave mode support
>
> 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.
Good point, I'll take this dependency out.
>
> > 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?
Nope, it's just that testing it is going to be slightly faster.
> You can use spi_controller_is_target() instead.
Will do.
>
> > + 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()
Will change.
>
> > + ret = wait_event_interruptible(csi->wait,
> > + ((csi->status & wait_mask) ==
> wait_mask) ||
> > + csi->errors || csi->slave_aborted);
>
> target_aborted (everywhere)
Will change.
Cheers,
Fab
>
> > + 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
