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

[Fabrizio Castro]

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