[PATCH 0/2] Add RZ/V2M CSI slave support

[PATCH 0/2] Add RZ/V2M CSI slave support

[Fabrizio Castro]

Dear All,

the CSI IP found inside the Renesas RZ/V2M SoC supports
both SPI master and slave.
This series extends the CSI dt-bindings and driver to
add SPI slave support.

Cheers,
Fab

Fabrizio Castro (2):
  spi: renesas,rzv2m-csi: Add SPI Slave related properties
  spi: rzv2m-csi: Add Slave mode support

 .../bindings/spi/renesas,rzv2m-csi.yaml       |  15 ++
 drivers/spi/Kconfig                           |   4 +-
 drivers/spi/spi-rzv2m-csi.c                   | 137 ++++++++++++------
 3 files changed, 112 insertions(+), 44 deletions(-)

-- 
2.34.1

[PATCH 1/2] spi: renesas,rzv2m-csi: Add SPI Slave related properties

[Fabrizio Castro]

The CSI IP found inside the Renesas RZ/V2M SoC can also work
in SPI slave mode.
When working in slave mode, the IP can make use of the SS
(Slave Select) pin, with "low" as default active level.
The active level of SS can be changed to "high" upon configuration.
This patch adds two new properties, one to make use of the
SS pin when in slave mode, and one to make the SS pin active high.

Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---
 .../bindings/spi/renesas,rzv2m-csi.yaml           | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/renesas,rzv2m-csi.yaml b/Documentation/devicetree/bindings/spi/renesas,rzv2m-csi.yaml
index e59183e53690..c3d8ad6525bb 100644
--- a/Documentation/devicetree/bindings/spi/renesas,rzv2m-csi.yaml
+++ b/Documentation/devicetree/bindings/spi/renesas,rzv2m-csi.yaml
@@ -39,6 +39,17 @@ properties:
   power-domains:
     maxItems: 1
 
+  renesas,csi-ss:
+    type: boolean
+    description:
+      Use CSI Slave Selection (SS) pin to enable transmission and reception when
+      in slave mode.
+
+  renesas,csi-ss-high:
+    type: boolean
+    description:
+      The SS pin is active high (by default the SS pin is active low).
+
 required:
   - compatible
   - reg
@@ -50,6 +61,10 @@ required:
   - '#address-cells'
   - '#size-cells'
 
+dependencies:
+  renesas,csi-ss: [ spi-slave ]
+  renesas,csi-ss-high: [ 'renesas,csi-ss' ]
+
 unevaluatedProperties: false
 
 examples:
-- 
2.34.1

Re: [PATCH 1/2] spi: renesas,rzv2m-csi: Add SPI Slave related properties

[Geert Uytterhoeven]

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 can also work
> in SPI slave mode.
> When working in slave mode, the IP can make use of the SS
> (Slave Select) pin, with "low" as default active level.
> The active level of SS can be changed to "high" upon configuration.
> This patch adds two new properties, one to make use of the
> SS pin when in slave mode, and one to make the SS pin active high.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>

Thanks for your patch!

> --- a/Documentation/devicetree/bindings/spi/renesas,rzv2m-csi.yaml
> +++ b/Documentation/devicetree/bindings/spi/renesas,rzv2m-csi.yaml
> @@ -39,6 +39,17 @@ properties:
>    power-domains:
>      maxItems: 1
>
> +  renesas,csi-ss:
> +    type: boolean
> +    description:
> +      Use CSI Slave Selection (SS) pin to enable transmission and reception when
> +      in slave mode.

Can't this be done in a more generic way? I had expected that the
existing SPI_NO_CS flag can be set using a property in the "slave" subnode,
but apparently there is no "spi-no-cs" property defined yet.

> +
> +  renesas,csi-ss-high:
> +    type: boolean
> +    description:
> +      The SS pin is active high (by default the SS pin is active low).

Can't you use the "spi-cs-high" property in the "slave" subnode instead?

> +
>  required:
>    - compatible
>    - reg

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

Re: [PATCH 1/2] spi: renesas,rzv2m-csi: Add SPI Slave related properties

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 807 bytes --]

On Wed, Sep 27, 2023 at 09:59:05AM +0200, Geert Uytterhoeven wrote:
> On Tue, Sep 26, 2023 at 11:08 PM Fabrizio Castro

> > +    type: boolean
> > +    description:
> > +      Use CSI Slave Selection (SS) pin to enable transmission and reception when
> > +      in slave mode.

> Can't this be done in a more generic way? I had expected that the
> existing SPI_NO_CS flag can be set using a property in the "slave" subnode,
> but apparently there is no "spi-no-cs" property defined yet.

The description is clearly saying there is a chip select, _NO_CS seems
entirely inappropriate.  It's not specified in the device tree because
when there's no chip select for a device it's a fundamental property of
how the device is controlled and we don't need any information beyond
the compatible.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] spi: renesas,rzv2m-csi: Add SPI Slave related properties

[Geert Uytterhoeven]

Hi Mark,

On Wed, Sep 27, 2023 at 11:00 AM Mark Brown <broonie@kernel.org> wrote:
> On Wed, Sep 27, 2023 at 09:59:05AM +0200, Geert Uytterhoeven wrote:
> > On Tue, Sep 26, 2023 at 11:08 PM Fabrizio Castro
> > > +    type: boolean
> > > +    description:
> > > +      Use CSI Slave Selection (SS) pin to enable transmission and reception when
> > > +      in slave mode.
>
> > Can't this be done in a more generic way? I had expected that the
> > existing SPI_NO_CS flag can be set using a property in the "slave" subnode,
> > but apparently there is no "spi-no-cs" property defined yet.
>
> The description is clearly saying there is a chip select, _NO_CS seems
> entirely inappropriate.  It's not specified in the device tree because
> when there's no chip select for a device it's a fundamental property of
> how the device is controlled and we don't need any information beyond
> the compatible.

In host mode, it indeed doesn't matter, as you can have only a single
device connected with SPI_NO_CS.
In device mode, the device needs to know if it must monitor the chip
select line or not.

In hindsight, I should have kept the question I had written initially,
but deleted after having read the documentation for the corresponding
RZ/V2M register bits:

    What does it mean if this is false? That there is no chip select?

So "spi-no-cs" would be the inverse of "renesas,csi-ss".

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

Re: [PATCH 1/2] spi: renesas,rzv2m-csi: Add SPI Slave related properties

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1299 bytes --]

On Wed, Sep 27, 2023 at 11:10:58AM +0200, Geert Uytterhoeven wrote:
> On Wed, Sep 27, 2023 at 11:00 AM Mark Brown <broonie@kernel.org> wrote:

> > The description is clearly saying there is a chip select, _NO_CS seems
> > entirely inappropriate.  It's not specified in the device tree because
> > when there's no chip select for a device it's a fundamental property of
> > how the device is controlled and we don't need any information beyond
> > the compatible.

> In host mode, it indeed doesn't matter, as you can have only a single
> device connected with SPI_NO_CS.
> In device mode, the device needs to know if it must monitor the chip
> select line or not.

> In hindsight, I should have kept the question I had written initially,
> but deleted after having read the documentation for the corresponding
> RZ/V2M register bits:

>     What does it mean if this is false? That there is no chip select?

> So "spi-no-cs" would be the inverse of "renesas,csi-ss".

I see.  Is there any control over what the chip select is when there is
one, in which case we could just look to see if there's one specified?

I'm a bit nervous about a generic property that maps onto _NO_CS since
it's likely that people will start using that in device bindings when
they shouldn't.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] spi: renesas,rzv2m-csi: Add SPI Slave related properties

[Geert Uytterhoeven]

Hi Mark,

On Wed, Sep 27, 2023 at 11:21 AM Mark Brown <broonie@kernel.org> wrote:
> On Wed, Sep 27, 2023 at 11:10:58AM +0200, Geert Uytterhoeven wrote:
> > On Wed, Sep 27, 2023 at 11:00 AM Mark Brown <broonie@kernel.org> wrote:
>
> > > The description is clearly saying there is a chip select, _NO_CS seems
> > > entirely inappropriate.  It's not specified in the device tree because
> > > when there's no chip select for a device it's a fundamental property of
> > > how the device is controlled and we don't need any information beyond
> > > the compatible.
>
> > In host mode, it indeed doesn't matter, as you can have only a single
> > device connected with SPI_NO_CS.
> > In device mode, the device needs to know if it must monitor the chip
> > select line or not.
>
> > In hindsight, I should have kept the question I had written initially,
> > but deleted after having read the documentation for the corresponding
> > RZ/V2M register bits:
>
> >     What does it mean if this is false? That there is no chip select?
>
> > So "spi-no-cs" would be the inverse of "renesas,csi-ss".
>
> I see.  Is there any control over what the chip select is when there is
> one, in which case we could just look to see if there's one specified?

On RZ/V2M there isn't, as there is only a single hardware chip select.

On MSIOF, there are 3 hardware chip selects, but apparently only the
primary one can be used in target mode.

I have to admit I never thought about this before (commit
cf9e4784f3bde3e4 ("spi: sh-msiof: Add slave mode support") also predates
commit 9cce882bedd2768d ("spi: sh-msiof: Extend support to 3 native chip
selects")).  Hence the SPI target DT bindings use a single "slave" subnode,
without a unit address, thus assuming no explicit (or a default)
chip select configuration.

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

Re: [PATCH 1/2] spi: renesas,rzv2m-csi: Add SPI Slave related properties

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 750 bytes --]

On Wed, Sep 27, 2023 at 11:44:17AM +0200, Geert Uytterhoeven wrote:
> On Wed, Sep 27, 2023 at 11:21 AM Mark Brown <broonie@kernel.org> wrote:

> > I see.  Is there any control over what the chip select is when there is
> > one, in which case we could just look to see if there's one specified?

> On RZ/V2M there isn't, as there is only a single hardware chip select.

> On MSIOF, there are 3 hardware chip selects, but apparently only the
> primary one can be used in target mode.

OK, it sounds like we do need a property then.  Like I say I'd rather
not have one that just works for _NO_CS in order to avoid confusion for
people writing SPI device drivers, either something in the generic
target binding or a device specific one.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH 1/2] spi: renesas,rzv2m-csi: Add SPI Slave related properties

[Fabrizio Castro]

Hi Mark,

Thanks for your reply!

> From: Mark Brown <broonie@kernel.org>
> Subject: Re: [PATCH 1/2] spi: renesas,rzv2m-csi: Add SPI Slave related
> properties
> 
> On Wed, Sep 27, 2023 at 11:44:17AM +0200, Geert Uytterhoeven wrote:
> > On Wed, Sep 27, 2023 at 11:21 AM Mark Brown <broonie@kernel.org>
> wrote:
> 
> > > I see.  Is there any control over what the chip select is when
> there is
> > > one, in which case we could just look to see if there's one
> specified?
> 
> > On RZ/V2M there isn't, as there is only a single hardware chip
> select.
> 
> > On MSIOF, there are 3 hardware chip selects, but apparently only the
> > primary one can be used in target mode.
> 
> OK, it sounds like we do need a property then.  Like I say I'd rather
> not have one that just works for _NO_CS in order to avoid confusion
> for
> people writing SPI device drivers, either something in the generic
> target binding or a device specific one.

Shall I invert the logic then? What I mean is I could drop property
"renesas,csi-ss" and add property "renesas,csi-no-ss" instead, therefore
without "renesas,csi-no-ss" pin SS will be used, with "renesas,csi-no-ss"
pin SS won't be used.
What do you think?

Also, I could drop "renesas,csi-ss-high" and use "spi-cs-high" instead?

Geert, any thoughts on the above?

Thanks,
Fab

Re: [PATCH 1/2] spi: renesas,rzv2m-csi: Add SPI Slave related properties

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 911 bytes --]

On Wed, Sep 27, 2023 at 10:18:57AM +0000, Fabrizio Castro wrote:
> > From: Mark Brown <broonie@kernel.org>

> > OK, it sounds like we do need a property then.  Like I say I'd rather
> > not have one that just works for _NO_CS in order to avoid confusion
> > for
> > people writing SPI device drivers, either something in the generic
> > target binding or a device specific one.

> Shall I invert the logic then? What I mean is I could drop property
> "renesas,csi-ss" and add property "renesas,csi-no-ss" instead, therefore
> without "renesas,csi-no-ss" pin SS will be used, with "renesas,csi-no-ss"
> pin SS won't be used.
> What do you think?

That sounds fine for me, I guess we could add a further property if some
new IP allows multiple options for the chip select in target mode.

> Also, I could drop "renesas,csi-ss-high" and use "spi-cs-high" instead?

I think that's OK but I looked less at that bit.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH 1/2] spi: renesas,rzv2m-csi: Add SPI Slave related properties

[Fabrizio Castro]

Hi Geert,

Thanks for your reply!

> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Subject: Re: [PATCH 1/2] spi: renesas,rzv2m-csi: Add SPI Slave related
> properties
> 
> Hi Mark,
> 
> On Wed, Sep 27, 2023 at 11:00 AM Mark Brown <broonie@kernel.org> wrote:
> > On Wed, Sep 27, 2023 at 09:59:05AM +0200, Geert Uytterhoeven wrote:
> > > On Tue, Sep 26, 2023 at 11:08 PM Fabrizio Castro
> > > > +    type: boolean
> > > > +    description:
> > > > +      Use CSI Slave Selection (SS) pin to enable transmission
> and reception when
> > > > +      in slave mode.
> >
> > > Can't this be done in a more generic way? I had expected that the
> > > existing SPI_NO_CS flag can be set using a property in the "slave"
> subnode,
> > > but apparently there is no "spi-no-cs" property defined yet.
> >
> > The description is clearly saying there is a chip select, _NO_CS
> seems
> > entirely inappropriate.  It's not specified in the device tree
> because
> > when there's no chip select for a device it's a fundamental property
> of
> > how the device is controlled and we don't need any information
> beyond
> > the compatible.
> 
> In host mode, it indeed doesn't matter, as you can have only a single
> device connected with SPI_NO_CS.
> In device mode, the device needs to know if it must monitor the chip
> select line or not.
> 
> In hindsight, I should have kept the question I had written initially,
> but deleted after having read the documentation for the corresponding
> RZ/V2M register bits:
> 
>     What does it mean if this is false? That there is no chip select?

Yes, that's what it means. The IP would just use the clock line to
shift data in and out.
Clearly, that could lead to "misunderstandings" as changing
the active level of the clock line on the host side may lead to 
extra clock cycles interpreted by the slave.

> 
> So "spi-no-cs" would be the inverse of "renesas,csi-ss".

Yes, exactly.

Cheers,
Fab

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

RE: [PATCH 1/2] spi: renesas,rzv2m-csi: Add SPI Slave related properties

[Fabrizio Castro]

Hi Geert,

Thanks for your reply!

> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Subject: Re: [PATCH 1/2] spi: renesas,rzv2m-csi: Add SPI Slave related
> properties
> 
> 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 can also work
> > in SPI slave mode.
> > When working in slave mode, the IP can make use of the SS
> > (Slave Select) pin, with "low" as default active level.
> > The active level of SS can be changed to "high" upon configuration.
> > This patch adds two new properties, one to make use of the
> > SS pin when in slave mode, and one to make the SS pin active high.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/Documentation/devicetree/bindings/spi/renesas,rzv2m-csi.yaml
> > +++ b/Documentation/devicetree/bindings/spi/renesas,rzv2m-csi.yaml
> > @@ -39,6 +39,17 @@ properties:
> >    power-domains:
> >      maxItems: 1
> >
> > +  renesas,csi-ss:
> > +    type: boolean
> > +    description:
> > +      Use CSI Slave Selection (SS) pin to enable transmission and
> reception when
> > +      in slave mode.
> 
> Can't this be done in a more generic way? I had expected that the
> existing SPI_NO_CS flag can be set using a property in the "slave"
> subnode,
> but apparently there is no "spi-no-cs" property defined yet.

I couldn't find a generic way to address that in a generic way.

> 
> > +
> > +  renesas,csi-ss-high:
> > +    type: boolean
> > +    description:
> > +      The SS pin is active high (by default the SS pin is active
> low).
> 
> Can't you use the "spi-cs-high" property in the "slave" subnode
> instead?

I could. I didn't go for it for the first version of this series
after reading the automatic handling of SPI_CS_HIGH (which for
example doesn't get automatically added to the mode for targets
using a GPIO as CS). Even though nothing prevents you from
explicitly adding SPI_CS_HIGH to the mode and using "spi-cs-high"
in the device tree, I thought that could be confusing, but
maybe it's not?

Thanks,
Fab

> 
> > +
> >  required:
> >    - compatible
> >    - reg
> 
> 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

Re: [PATCH 1/2] spi: renesas,rzv2m-csi: Add SPI Slave related properties

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 783 bytes --]

On Tue, Sep 26, 2023 at 10:08:17PM +0100, Fabrizio Castro wrote:
> The CSI IP found inside the Renesas RZ/V2M SoC can also work
> in SPI slave mode.
> When working in slave mode, the IP can make use of the SS
> (Slave Select) pin, with "low" as default active level.
> The active level of SS can be changed to "high" upon configuration.
> This patch adds two new properties, one to make use of the
> SS pin when in slave mode, and one to make the SS pin active high.

Please avoid the use of outdated terminology like this, prefer "device
mode" or similar.

> +  renesas,csi-ss:
> +    type: boolean
> +    description:
> +      Use CSI Slave Selection (SS) pin to enable transmission and reception when
> +      in slave mode.

When would this ever not be true when in device mode?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH 1/2] spi: renesas,rzv2m-csi: Add SPI Slave related properties

[Fabrizio Castro]

Hi Mark,

Thanks for your reply!

> From: Mark Brown <broonie@kernel.org>
> Subject: Re: [PATCH 1/2] spi: renesas,rzv2m-csi: Add SPI Slave related
> properties
> 
> On Tue, Sep 26, 2023 at 10:08:17PM +0100, Fabrizio Castro wrote:
> > The CSI IP found inside the Renesas RZ/V2M SoC can also work
> > in SPI slave mode.
> > When working in slave mode, the IP can make use of the SS
> > (Slave Select) pin, with "low" as default active level.
> > The active level of SS can be changed to "high" upon configuration.
> > This patch adds two new properties, one to make use of the
> > SS pin when in slave mode, and one to make the SS pin active high.
> 
> Please avoid the use of outdated terminology like this, prefer "device
> mode" or similar.

Okay, I'll resend with a more modern terminology ("host" and "target").


> 
> > +  renesas,csi-ss:
> > +    type: boolean
> > +    description:
> > +      Use CSI Slave Selection (SS) pin to enable transmission and
> reception when
> > +      in slave mode.
> 
> When would this ever not be true when in device mode?


This IP can do without the SS pin to start operations. In fact, the
SS pin is disabled by default (I know...), it needs to be explicitly
enabled, and when enabled the pin is active low by default.

Cheers,
Fab

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

[Fabrizio Castro]

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>
---
 drivers/spi/Kconfig         |   4 +-
 drivers/spi/spi-rzv2m-csi.c | 137 +++++++++++++++++++++++++-----------
 2 files changed, 97 insertions(+), 44 deletions(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 2c21d5b96fdc..2c657df3e304 100644
--- 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
 	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"
diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c
index d0f51b17aa7c..c700a9922402 100644
--- a/drivers/spi/spi-rzv2m-csi.c
+++ b/drivers/spi/spi-rzv2m-csi.c
@@ -11,6 +11,7 @@
 #include <linux/interrupt.h>
 #include <linux/iopoll.h>
 #include <linux/log2.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
 #include <linux/reset.h>
@@ -38,6 +39,9 @@
 #define CSI_MODE_SETUP		0x00000040
 
 /* CSI_CLKSEL */
+#define CSI_CLKSEL_SS_ENA	BIT(19)
+#define CSI_CLKSEL_SS_POL	BIT(18)
+#define CSI_CLKSEL_SS		(CSI_CLKSEL_SS_ENA | CSI_CLKSEL_SS_POL)
 #define CSI_CLKSEL_CKP		BIT(17)
 #define CSI_CLKSEL_DAP		BIT(16)
 #define CSI_CLKSEL_MODE		(CSI_CLKSEL_CKP|CSI_CLKSEL_DAP)
@@ -82,6 +86,15 @@
 
 #define CSI_MAX_SPI_SCKO	(8 * HZ_PER_MHZ)
 
+#define CSI_CLKSEL_SS_DISABLED			0
+#define CSI_CLKSEL_SS_ENABLED_ACTIVE_LOW	BIT(1)
+#define CSI_CLKSEL_SS_ENABLED_ACTIVE_HIGH	GENMASK(1, 0)
+
+enum {
+	RZV2M_CSI_SPI_MASTER,
+	RZV2M_CSI_SPI_SLAVE,
+};
+
 struct rzv2m_csi_priv {
 	void __iomem *base;
 	struct clk *csiclk;
@@ -99,6 +112,9 @@ struct rzv2m_csi_priv {
 	wait_queue_head_t wait;
 	u32 errors;
 	u32 status;
+	int mode;
+	int slave_select;
+	bool slave_aborted;
 };
 
 static void rzv2m_csi_reg_write_bit(const struct rzv2m_csi_priv *csi,
@@ -193,6 +209,14 @@ static int rzv2m_csi_read_rxfifo(struct rzv2m_csi_priv *csi)
 	return 0;
 }
 
+static inline void rzv2m_csi_empty_rxfifo(struct rzv2m_csi_priv *csi)
+{
+	unsigned int i;
+
+	for (i = 0; i < csi->words_to_transfer; i++)
+		readl(csi->base + CSI_IFIFO);
+}
+
 static inline void rzv2m_csi_calc_current_transfer(struct rzv2m_csi_priv *csi)
 {
 	unsigned int bytes_transferred = max(csi->bytes_received, csi->bytes_sent);
@@ -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) {
+		ret = wait_event_interruptible(csi->wait,
+				((csi->status & wait_mask) == wait_mask) ||
+				csi->errors || csi->slave_aborted);
+		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;
+	}
 
 	rzv2m_csi_disable_irqs(csi, enable_bits);
 
 	if (csi->errors)
 		return -EIO;
 
-	if (!ret)
-		return -ETIMEDOUT;
-
-	return 0;
-}
-
-static int rzv2m_csi_wait_for_tx_empty(struct rzv2m_csi_priv *csi)
-{
-	int ret;
-
-	if (readl(csi->base + CSI_OFIFOL) == 0)
-		return 0;
-
-	ret = rzv2m_csi_wait_for_interrupt(csi, CSI_INT_TREND, CSI_CNT_TREND_E);
-	if (ret == -ETIMEDOUT)
-		csi->errors |= TX_TIMEOUT_ERROR;
-
 	return ret;
 }
 
@@ -312,7 +327,7 @@ static inline int rzv2m_csi_wait_for_rx_ready(struct rzv2m_csi_priv *csi)
 {
 	int ret;
 
-	if (readl(csi->base + CSI_IFIFOL) == csi->bytes_to_transfer)
+	if (readl(csi->base + CSI_IFIFOL) >= csi->bytes_to_transfer)
 		return 0;
 
 	ret = rzv2m_csi_wait_for_interrupt(csi, CSI_INT_R_TRGR,
@@ -402,8 +417,14 @@ static int rzv2m_csi_setup(struct spi_device *spi)
 	rzv2m_csi_reg_write_bit(csi, CSI_MODE, CSI_MODE_DIR,
 				!!(spi->mode & SPI_LSB_FIRST));
 
-	/* Set the operation mode as master */
-	rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_SLAVE, 0);
+	/* Set the role, 1 for slave and 0 for master */
+	rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_SLAVE,
+				csi->mode == RZV2M_CSI_SPI_SLAVE);
+
+	if (csi->mode == RZV2M_CSI_SPI_SLAVE)
+		/* Configure the slave select pin */
+		rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_SS,
+					csi->slave_select);
 
 	/* Give the IP a SW reset */
 	ret = rzv2m_csi_sw_reset(csi, 1);
@@ -431,9 +452,13 @@ static int rzv2m_csi_pio_transfer(struct rzv2m_csi_priv *csi)
 	/* Make sure the TX FIFO is empty */
 	writel(0, csi->base + CSI_OFIFOL);
 
+	/* Make sure the RX FIFO is empty */
+	writel(0, csi->base + CSI_IFIFOL);
+
 	csi->bytes_sent = 0;
 	csi->bytes_received = 0;
 	csi->errors = 0;
+	csi->slave_aborted = false;
 
 	rzv2m_csi_disable_all_irqs(csi);
 	rzv2m_csi_clear_all_irqs(csi);
@@ -452,28 +477,21 @@ static int rzv2m_csi_pio_transfer(struct rzv2m_csi_priv *csi)
 
 		rzv2m_csi_enable_irqs(csi, CSI_INT_OVERF | CSI_INT_UNDER);
 
-		/* Make sure the RX FIFO is empty */
-		writel(0, csi->base + CSI_IFIFOL);
-
 		writel(readl(csi->base + CSI_INT), csi->base + CSI_INT);
 		csi->status = 0;
 
-		rzv2m_csi_start_stop_operation(csi, 1, false);
-
 		/* TX */
 		if (csi->txbuf) {
 			ret = rzv2m_csi_fill_txfifo(csi);
 			if (ret)
 				break;
 
-			ret = rzv2m_csi_wait_for_tx_empty(csi);
-			if (ret)
-				break;
-
 			if (csi->bytes_sent == csi->buffer_len)
 				tx_completed = true;
 		}
 
+		rzv2m_csi_start_stop_operation(csi, 1, false);
+
 		/*
 		 * Make sure the RX FIFO contains the desired number of words.
 		 * We then either flush its content, or we copy it onto
@@ -483,31 +501,28 @@ static int rzv2m_csi_pio_transfer(struct rzv2m_csi_priv *csi)
 		if (ret)
 			break;
 
-		/* RX */
-		if (csi->rxbuf) {
+		if (csi->mode == RZV2M_CSI_SPI_MASTER)
 			rzv2m_csi_start_stop_operation(csi, 0, false);
 
+		/* RX */
+		if (csi->rxbuf) {
 			ret = rzv2m_csi_read_rxfifo(csi);
 			if (ret)
 				break;
 
 			if (csi->bytes_received == csi->buffer_len)
 				rx_completed = true;
+		} else {
+			rzv2m_csi_empty_rxfifo(csi);
 		}
 
-		ret = rzv2m_csi_start_stop_operation(csi, 0, true);
-		if (ret)
-			goto pio_quit;
-
 		if (csi->errors) {
 			ret = -EIO;
-			goto pio_quit;
+			break;
 		}
 	}
 
 	rzv2m_csi_start_stop_operation(csi, 0, true);
-
-pio_quit:
 	rzv2m_csi_disable_all_irqs(csi);
 	rzv2m_csi_enable_rx_trigger(csi, false);
 	rzv2m_csi_clear_all_irqs(csi);
@@ -529,7 +544,8 @@ static int rzv2m_csi_transfer_one(struct spi_controller *controller,
 
 	rzv2m_csi_setup_operating_mode(csi, transfer);
 
-	rzv2m_csi_setup_clock(csi, transfer->speed_hz);
+	if (csi->mode == RZV2M_CSI_SPI_MASTER)
+		rzv2m_csi_setup_clock(csi, transfer->speed_hz);
 
 	ret = rzv2m_csi_pio_transfer(csi);
 	if (ret) {
@@ -546,24 +562,58 @@ static int rzv2m_csi_transfer_one(struct spi_controller *controller,
 	return ret;
 }
 
+static int rzv2m_csi_slave_abort(struct spi_controller *ctlr)
+{
+	struct rzv2m_csi_priv *csi = spi_controller_get_devdata(ctlr);
+
+	csi->slave_aborted = true;
+	wake_up(&csi->wait);
+
+	return 0;
+}
+
 static int rzv2m_csi_probe(struct platform_device *pdev)
 {
+	struct device_node *np = pdev->dev.of_node;
 	struct spi_controller *controller;
 	struct device *dev = &pdev->dev;
 	struct rzv2m_csi_priv *csi;
 	struct reset_control *rstc;
+	int mode;
 	int irq;
 	int ret;
 
-	controller = devm_spi_alloc_host(dev, sizeof(*csi));
+	mode = of_property_read_bool(np, "spi-slave") ? RZV2M_CSI_SPI_SLAVE :
+							RZV2M_CSI_SPI_MASTER;
+
+	if (mode == RZV2M_CSI_SPI_MASTER)
+		controller = devm_spi_alloc_host(dev, sizeof(*csi));
+	else
+		controller = devm_spi_alloc_target(dev, sizeof(*csi));
+
 	if (!controller)
 		return -ENOMEM;
 
 	csi = spi_controller_get_devdata(controller);
 	platform_set_drvdata(pdev, csi);
 
+	if (mode == RZV2M_CSI_SPI_SLAVE) {
+		if (of_property_read_bool(np, "renesas,csi-ss")) {
+			if (of_property_read_bool(np, "renesas,csi-ss-high"))
+				csi->slave_select =
+					CSI_CLKSEL_SS_ENABLED_ACTIVE_HIGH;
+			else
+				csi->slave_select =
+					CSI_CLKSEL_SS_ENABLED_ACTIVE_LOW;
+		} else {
+			csi->slave_select = CSI_CLKSEL_SS_DISABLED;
+		}
+	}
+
 	csi->dev = dev;
 	csi->controller = controller;
+	csi->mode = mode;
+	csi->slave_aborted = false;
 
 	csi->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(csi->base))
@@ -594,6 +644,7 @@ static int rzv2m_csi_probe(struct platform_device *pdev)
 	controller->setup = rzv2m_csi_setup;
 	controller->transfer_one = rzv2m_csi_transfer_one;
 	controller->use_gpio_descriptors = true;
+	controller->slave_abort = rzv2m_csi_slave_abort;
 
 	device_set_node(&controller->dev, dev_fwnode(dev));
 
-- 
2.34.1

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 194 bytes --]

On Tue, Sep 26, 2023 at 10:08:18PM +0100, Fabrizio Castro wrote:

> The CSI IP found inside the Renesas RZ/V2M SoC supports
> both SPI Master and SPI Slave roles.

Prefer controller and device.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Geert Uytterhoeven]

Hi Mark,

On Wed, Sep 27, 2023 at 11:02 AM Mark Brown <broonie@kernel.org> wrote:
> On Tue, Sep 26, 2023 at 10:08:18PM +0100, Fabrizio Castro wrote:
>
> > The CSI IP found inside the Renesas RZ/V2M SoC supports
> > both SPI Master and SPI Slave roles.
>
> Prefer controller and device.

You mean host and target?
(oops, got the latter wrong in my previous email, too ;-)

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 361 bytes --]

On Wed, Sep 27, 2023 at 11:12:49AM +0200, Geert Uytterhoeven wrote:
> On Wed, Sep 27, 2023 at 11:02 AM Mark Brown <broonie@kernel.org> wrote:

> > Prefer controller and device.

> You mean host and target?
> (oops, got the latter wrong in my previous email, too ;-)

Those also work, yes and there's less chance of confusion for the
controller bit.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

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

[Fabrizio Castro]

Hi Geert,

Thanks for your reply!

> From: Geert Uytterhoeven <geert@linux-m68k.org>
> 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@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.

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