[PATCH] spi: spi-rspi: Add force_dma variable to spi_ops

[PATCH] spi: spi-rspi: Add force_dma variable to spi_ops

[Biju Das]

On RZ/G2L SoCs switching from DMA to interrupt mode, causes timeout
issue as we are not getting Rx interrupt even though SPRF bit is set in
the status register.

But there is no issue if we don't switch between interrupt to DMA mode
or vice versa.

Performance comparison between interrupt and DMA mode on RZ/Five SMARC
platform connected to a display module shows that performance and
CPU utilization is much better with DMA mode compared to interrupt mode
(1->65 fps) and (98->8%).

This patch introduces a variable force_dma to avoid switching between
DMA to interrupt mode for RZ platforms.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/spi/spi-rspi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c
index 7a014eeec2d0..f86b7b53288f 100644
--- a/drivers/spi/spi-rspi.c
+++ b/drivers/spi/spi-rspi.c
@@ -249,6 +249,7 @@ struct spi_ops {
 	u16 flags;
 	u16 fifo_size;
 	u8 num_hw_ss;
+	bool force_dma;
 };
 
 static void rspi_set_rate(struct rspi_data *rspi)
@@ -677,7 +678,7 @@ static void qspi_receive_init(const struct rspi_data *rspi)
 static bool __rspi_can_dma(const struct rspi_data *rspi,
 			   const struct spi_transfer *xfer)
 {
-	return xfer->len > rspi->ops->fifo_size;
+	return  rspi->ops->force_dma || (xfer->len > rspi->ops->fifo_size);
 }
 
 static bool rspi_can_dma(struct spi_controller *ctlr, struct spi_device *spi,
@@ -1196,6 +1197,7 @@ static const struct spi_ops rspi_rz_ops = {
 	.flags =		SPI_CONTROLLER_MUST_RX | SPI_CONTROLLER_MUST_TX,
 	.fifo_size =		8,	/* 8 for TX, 32 for RX */
 	.num_hw_ss =		1,
+	.force_dma =		true,
 };
 
 static const struct spi_ops qspi_ops = {
-- 
2.25.1

Re: [PATCH] spi: spi-rspi: Add force_dma variable to spi_ops

[Mark Brown]

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

On Sat, Jul 16, 2022 at 04:39:34PM +0100, Biju Das wrote:
> On RZ/G2L SoCs switching from DMA to interrupt mode, causes timeout
> issue as we are not getting Rx interrupt even though SPRF bit is set in
> the status register.
> 
> But there is no issue if we don't switch between interrupt to DMA mode
> or vice versa.

So we need this for correctness until someone can figure out what goes
wrong :/

> Performance comparison between interrupt and DMA mode on RZ/Five SMARC
> platform connected to a display module shows that performance and
> CPU utilization is much better with DMA mode compared to interrupt mode
> (1->65 fps) and (98->8%).

How does that look for something doing lots of short transfers (like
register I/O for example), and what's the throughput like?  DMA probably
is the most sensible default even so since the impact of doing PIO for
large transfers tends to be so bad, you have to be doing a *lot* of
small I/O before it gets to be a problem whereas basically any large
transfer will notice PIO.

> This patch introduces a variable force_dma to avoid switching between
> DMA to interrupt mode for RZ platforms.

Not that it really matters but it's not a variable, it's a flag in the
device configuration.  I'm wondering if we might want a way to override
this at runtime in case someone does have an application that suffers
badly from being forced into DMA (eg, some IIO thing), that could be
done incrementally though.

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

RE: [PATCH] spi: spi-rspi: Add force_dma variable to spi_ops

[Biju Das]

Hi Mark,

Thanks for the feedback.

> Subject: Re: [PATCH] spi: spi-rspi: Add force_dma variable to spi_ops
> 
> On Sat, Jul 16, 2022 at 04:39:34PM +0100, Biju Das wrote:
> > On RZ/G2L SoCs switching from DMA to interrupt mode, causes timeout
> > issue as we are not getting Rx interrupt even though SPRF bit is set
> > in the status register.
> >
> > But there is no issue if we don't switch between interrupt to DMA mode
> > or vice versa.
> 
> So we need this for correctness until someone can figure out what goes
> wrong :/

Yes, I am in touch with our HW engineers for the missing interrupt when
we switch from DMA to interrupt mode. I will update you once, I receive the
feedback from them.

> 
> > Performance comparison between interrupt and DMA mode on RZ/Five SMARC
> > platform connected to a display module shows that performance and CPU
> > utilization is much better with DMA mode compared to interrupt mode
> > (1->65 fps) and (98->8%).
> 
> How does that look for something doing lots of short transfers (like
> register I/O for example), and what's the throughput like?  DMA probably
> is the most sensible default even so since the impact of doing PIO for
> large transfers tends to be so bad, you have to be doing a *lot* of small
> I/O before it gets to be a problem whereas basically any large transfer
> will notice PIO.

Basically, this performance measurement done by running LVGL on RZ/Five
Connected to display module ili9341 by SPI interface.

Currently we are switch to DMA mode for transfer length greater than 8 bytes. 
Since we have an issue with DMA to interrupt mode switching, initially we tried with PIO mode and fps was very low 1 fps. So we switched to DMA mode and it shoots up the fps to 65.

Similar experiment done on RZ/G2UL connected to PMOD spi flash. In this case

1) The number of interrupts with PIO mode is 6 times greater than that of DMA.

2) The rd/wr test with DMA mode takes 3 minutes to finish whereas with PIO
mode it takes 4 minutes to finish.

> 
> > This patch introduces a variable force_dma to avoid switching between
> > DMA to interrupt mode for RZ platforms.
> 
> Not that it really matters but it's not a variable, it's a flag in the
> device configuration.  
OK.

> I'm wondering if we might want a way to override
> this at runtime in case someone does have an application that suffers
> badly from being forced into DMA (eg, some IIO thing), that could be done
> incrementally though.

OK. Sure. 

Cheers,
Biju

Re: [PATCH] spi: spi-rspi: Add force_dma variable to spi_ops

[Geert Uytterhoeven]

Hi Biju,

On Sat, Jul 16, 2022 at 5:39 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> On RZ/G2L SoCs switching from DMA to interrupt mode, causes timeout
> issue as we are not getting Rx interrupt even though SPRF bit is set in
> the status register.
>
> But there is no issue if we don't switch between interrupt to DMA mode
> or vice versa.
>
> Performance comparison between interrupt and DMA mode on RZ/Five SMARC
> platform connected to a display module shows that performance and
> CPU utilization is much better with DMA mode compared to interrupt mode
> (1->65 fps) and (98->8%).
>
> This patch introduces a variable force_dma to avoid switching between
> DMA to interrupt mode for RZ platforms.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch!

> @@ -1196,6 +1197,7 @@ static const struct spi_ops rspi_rz_ops = {
>         .flags =                SPI_CONTROLLER_MUST_RX | SPI_CONTROLLER_MUST_TX,
>         .fifo_size =            8,      /* 8 for TX, 32 for RX */
>         .num_hw_ss =            1,
> +       .force_dma =            true,
>  };

Do you know if this is needed on RZ/A series, too?
I know upstream does not have DMA support for RZ/A yet, but the
BSP has?

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] spi: spi-rspi: Add force_dma variable to spi_ops

[Biju Das]

Hi Geert,

+ Chris,

> Subject: Re: [PATCH] spi: spi-rspi: Add force_dma variable to spi_ops
> 
> Hi Biju,
> 
> On Sat, Jul 16, 2022 at 5:39 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > On RZ/G2L SoCs switching from DMA to interrupt mode, causes timeout
> > issue as we are not getting Rx interrupt even though SPRF bit is set
> > in the status register.
> >
> > But there is no issue if we don't switch between interrupt to DMA mode
> > or vice versa.
> >
> > Performance comparison between interrupt and DMA mode on RZ/Five SMARC
> > platform connected to a display module shows that performance and CPU
> > utilization is much better with DMA mode compared to interrupt mode
> > (1->65 fps) and (98->8%).
> >
> > This patch introduces a variable force_dma to avoid switching between
> > DMA to interrupt mode for RZ platforms.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > @@ -1196,6 +1197,7 @@ static const struct spi_ops rspi_rz_ops = {
> >         .flags =                SPI_CONTROLLER_MUST_RX |
> SPI_CONTROLLER_MUST_TX,
> >         .fifo_size =            8,      /* 8 for TX, 32 for RX */
> >         .num_hw_ss =            1,
> > +       .force_dma =            true,
> >  };
> 
> Do you know if this is needed on RZ/A series, too?

I guess it is needed?? I may be wrong. I got a link from Chris [1]. As per this still
We haven't found a solution. May be the priority is changed for this activity and
no one looked after this.

[1] https://lore.kernel.org/linux-renesas-soc/?q=spi-rspi+mixes+DMA+and+PIO+transfers+causing+PIO+transfer+to+fail

> I knowupstream does not have DMA support for RZ/A yet, but the BSP has?

RZ/G2L DMA driver is derived from RZ/A, with small changes it can support RZ/A.
I am not sure is there any plan to upstream DMA support for RZ/A?
I do not have RZ/A board.

Cheers,
Biju

Re: [PATCH] spi: spi-rspi: Add force_dma variable to spi_ops

[Geert Uytterhoeven]

Hi Biju,

On Tue, Jul 19, 2022 at 10:29 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH] spi: spi-rspi: Add force_dma variable to spi_ops
> > On Sat, Jul 16, 2022 at 5:39 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > On RZ/G2L SoCs switching from DMA to interrupt mode, causes timeout
> > > issue as we are not getting Rx interrupt even though SPRF bit is set
> > > in the status register.
> > >
> > > But there is no issue if we don't switch between interrupt to DMA mode
> > > or vice versa.
> > >
> > > Performance comparison between interrupt and DMA mode on RZ/Five SMARC
> > > platform connected to a display module shows that performance and CPU
> > > utilization is much better with DMA mode compared to interrupt mode
> > > (1->65 fps) and (98->8%).
> > >
> > > This patch introduces a variable force_dma to avoid switching between
> > > DMA to interrupt mode for RZ platforms.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> >
> > Thanks for your patch!
> >
> > > @@ -1196,6 +1197,7 @@ static const struct spi_ops rspi_rz_ops = {
> > >         .flags =                SPI_CONTROLLER_MUST_RX |
> > SPI_CONTROLLER_MUST_TX,
> > >         .fifo_size =            8,      /* 8 for TX, 32 for RX */
> > >         .num_hw_ss =            1,
> > > +       .force_dma =            true,
> > >  };
> >
> > Do you know if this is needed on RZ/A series, too?
>
> I guess it is needed?? I may be wrong. I got a link from Chris [1]. As per this still
> We haven't found a solution. May be the priority is changed for this activity and
> no one looked after this.
>
> [1] https://lore.kernel.org/linux-renesas-soc/?q=spi-rspi+mixes+DMA+and+PIO+transfers+causing+PIO+transfer+to+fail

Daniel said he found the issue, i.e. the dmac driver never resetting
DMARS?

> > I knowupstream does not have DMA support for RZ/A yet, but the BSP has?
>
> RZ/G2L DMA driver is derived from RZ/A, with small changes it can support RZ/A.
> I am not sure is there any plan to upstream DMA support for RZ/A?
> I do not have RZ/A board.

I have, so it might happen, one day...

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] spi: spi-rspi: Add force_dma variable to spi_ops

[Biju Das]

Hi Geert,

+Vinod

> Subject: Re: [PATCH] spi: spi-rspi: Add force_dma variable to spi_ops
> 
> Hi Biju,
> 
> On Tue, Jul 19, 2022 at 10:29 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > Subject: Re: [PATCH] spi: spi-rspi: Add force_dma variable to
> > > spi_ops On Sat, Jul 16, 2022 at 5:39 PM Biju Das
> <biju.das.jz@bp.renesas.com> wrote:
> > > > On RZ/G2L SoCs switching from DMA to interrupt mode, causes
> > > > timeout issue as we are not getting Rx interrupt even though SPRF
> > > > bit is set in the status register.
> > > >
> > > > But there is no issue if we don't switch between interrupt to DMA
> > > > mode or vice versa.
> > > >
> > > > Performance comparison between interrupt and DMA mode on RZ/Five
> > > > SMARC platform connected to a display module shows that
> > > > performance and CPU utilization is much better with DMA mode
> > > > compared to interrupt mode
> > > > (1->65 fps) and (98->8%).
> > > >
> > > > This patch introduces a variable force_dma to avoid switching
> > > > between DMA to interrupt mode for RZ platforms.
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > >
> > > Thanks for your patch!
> > >
> > > > @@ -1196,6 +1197,7 @@ static const struct spi_ops rspi_rz_ops = {
> > > >         .flags =                SPI_CONTROLLER_MUST_RX |
> > > SPI_CONTROLLER_MUST_TX,
> > > >         .fifo_size =            8,      /* 8 for TX, 32 for RX */
> > > >         .num_hw_ss =            1,
> > > > +       .force_dma =            true,
> > > >  };
> > >
> > > Do you know if this is needed on RZ/A series, too?
> >
> > I guess it is needed?? I may be wrong. I got a link from Chris [1]. As
> > per this still We haven't found a solution. May be the priority is
> > changed for this activity and no one looked after this.
> >
> > [1]
> 
> Daniel said he found the issue, i.e. the dmac driver never resetting DMARS?

Currently DMARS set during prepare and it never cleared. So I added device_synchronize
callback in DMA driver to clear the DMARS. RSPI client driver after synchronizing
DMA callback, calls dmaengine_synchronize which clears DMARS.

With this DMA to interrupt mode switching is working fine.

Cheers,
Biju

Re: [PATCH] spi: spi-rspi: Add force_dma variable to spi_ops

[Vinod Koul]

On 19-07-22, 11:28, Biju Das wrote:
> Hi Geert,
> 
> +Vinod
> 
> > Subject: Re: [PATCH] spi: spi-rspi: Add force_dma variable to spi_ops
> > 
> > Hi Biju,
> > 
> > On Tue, Jul 19, 2022 at 10:29 AM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > > Subject: Re: [PATCH] spi: spi-rspi: Add force_dma variable to
> > > > spi_ops On Sat, Jul 16, 2022 at 5:39 PM Biju Das
> > <biju.das.jz@bp.renesas.com> wrote:
> > > > > On RZ/G2L SoCs switching from DMA to interrupt mode, causes
> > > > > timeout issue as we are not getting Rx interrupt even though SPRF
> > > > > bit is set in the status register.
> > > > >
> > > > > But there is no issue if we don't switch between interrupt to DMA
> > > > > mode or vice versa.
> > > > >
> > > > > Performance comparison between interrupt and DMA mode on RZ/Five
> > > > > SMARC platform connected to a display module shows that
> > > > > performance and CPU utilization is much better with DMA mode
> > > > > compared to interrupt mode
> > > > > (1->65 fps) and (98->8%).
> > > > >
> > > > > This patch introduces a variable force_dma to avoid switching
> > > > > between DMA to interrupt mode for RZ platforms.

Why do you need a variable for that, if DMA is availble (you were able
to allocate channels) then use DMA, otherwise fall back to PIO..

Or anything missing from context which I am not aware of?

> > > > >
> > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > >
> > > > Thanks for your patch!
> > > >
> > > > > @@ -1196,6 +1197,7 @@ static const struct spi_ops rspi_rz_ops = {
> > > > >         .flags =                SPI_CONTROLLER_MUST_RX |
> > > > SPI_CONTROLLER_MUST_TX,
> > > > >         .fifo_size =            8,      /* 8 for TX, 32 for RX */
> > > > >         .num_hw_ss =            1,
> > > > > +       .force_dma =            true,
> > > > >  };
> > > >
> > > > Do you know if this is needed on RZ/A series, too?
> > >
> > > I guess it is needed?? I may be wrong. I got a link from Chris [1]. As
> > > per this still We haven't found a solution. May be the priority is
> > > changed for this activity and no one looked after this.
> > >
> > > [1]
> > 
> > Daniel said he found the issue, i.e. the dmac driver never resetting DMARS?
> 
> Currently DMARS set during prepare and it never cleared. So I added device_synchronize
> callback in DMA driver to clear the DMARS. RSPI client driver after synchronizing
> DMA callback, calls dmaengine_synchronize which clears DMARS.
> 
> With this DMA to interrupt mode switching is working fine.
> 
> Cheers,
> Biju
> 

-- 
~Vinod

RE: [PATCH] spi: spi-rspi: Add force_dma variable to spi_ops

[Biju Das]

Hi Vinod,

> Subject: Re: [PATCH] spi: spi-rspi: Add force_dma variable to spi_ops
> 
> On 19-07-22, 11:28, Biju Das wrote:
> > Hi Geert,
> >
> > +Vinod
> >
> > > Subject: Re: [PATCH] spi: spi-rspi: Add force_dma variable to
> > > spi_ops
> > >
> > > Hi Biju,
> > >
> > > On Tue, Jul 19, 2022 at 10:29 AM Biju Das
> > > <biju.das.jz@bp.renesas.com>
> > > wrote:
> > > > > Subject: Re: [PATCH] spi: spi-rspi: Add force_dma variable to
> > > > > spi_ops On Sat, Jul 16, 2022 at 5:39 PM Biju Das
> > > <biju.das.jz@bp.renesas.com> wrote:
> > > > > > On RZ/G2L SoCs switching from DMA to interrupt mode, causes
> > > > > > timeout issue as we are not getting Rx interrupt even though
> > > > > > SPRF bit is set in the status register.
> > > > > >
> > > > > > But there is no issue if we don't switch between interrupt to
> > > > > > DMA mode or vice versa.
> > > > > >
> > > > > > Performance comparison between interrupt and DMA mode on
> > > > > > RZ/Five SMARC platform connected to a display module shows
> > > > > > that performance and CPU utilization is much better with DMA
> > > > > > mode compared to interrupt mode
> > > > > > (1->65 fps) and (98->8%).
> > > > > >
> > > > > > This patch introduces a variable force_dma to avoid switching
> > > > > > between DMA to interrupt mode for RZ platforms.
> 
> Why do you need a variable for that, if DMA is availble (you were able to
> allocate channels) then use DMA, otherwise fall back to PIO..

I was using DMA. We are not getting rspi interrupts after the DMA to PIO switch
because of [1]. ie, we are not clearing DMAR in DMA driver
and interrupt requests to the interrupt controller are masked.

[1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220719150000.383722-2-biju.das.jz@bp.renesas.com/

> 
> Or anything missing from context which I am not aware of?

After this discussion, I have posted [1] and [2] to fix this issue.

[2] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220719150000.383722-1-biju.das.jz@bp.renesas.com/

Cheers,
Biju


> 
> > > > > >
> > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > >
> > > > > Thanks for your patch!
> > > > >
> > > > > > @@ -1196,6 +1197,7 @@ static const struct spi_ops rspi_rz_ops = {
> > > > > >         .flags =                SPI_CONTROLLER_MUST_RX |
> > > > > SPI_CONTROLLER_MUST_TX,
> > > > > >         .fifo_size =            8,      /* 8 for TX, 32 for RX */
> > > > > >         .num_hw_ss =            1,
> > > > > > +       .force_dma =            true,
> > > > > >  };
> > > > >
> > > > > Do you know if this is needed on RZ/A series, too?
> > > >
> > > > I guess it is needed?? I may be wrong. I got a link from Chris
> > > > [1]. As per this still We haven't found a solution. May be the
> > > > priority is changed for this activity and no one looked after this.
> > > >
> > > > [1]
> > >
> > > Daniel said he found the issue, i.e. the dmac driver never resetting
> DMARS?
> >
> > Currently DMARS set during prepare and it never cleared. So I added
> > device_synchronize callback in DMA driver to clear the DMARS. RSPI
> > client driver after synchronizing DMA callback, calls
> dmaengine_synchronize which clears DMARS.
> >
> > With this DMA to interrupt mode switching is working fine.
> >
> > Cheers,
> > Biju
> >
> 
> --
> ~Vinod

Re: [PATCH] spi: spi-rspi: Add force_dma variable to spi_ops

[Vinod Koul]

On 20-07-22, 05:13, Biju Das wrote:
> Hi Vinod,
> 
> > Subject: Re: [PATCH] spi: spi-rspi: Add force_dma variable to spi_ops
> > 
> > On 19-07-22, 11:28, Biju Das wrote:
> > > Hi Geert,
> > >
> > > +Vinod
> > >
> > > > Subject: Re: [PATCH] spi: spi-rspi: Add force_dma variable to
> > > > spi_ops
> > > >
> > > > Hi Biju,
> > > >
> > > > On Tue, Jul 19, 2022 at 10:29 AM Biju Das
> > > > <biju.das.jz@bp.renesas.com>
> > > > wrote:
> > > > > > Subject: Re: [PATCH] spi: spi-rspi: Add force_dma variable to
> > > > > > spi_ops On Sat, Jul 16, 2022 at 5:39 PM Biju Das
> > > > <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > On RZ/G2L SoCs switching from DMA to interrupt mode, causes
> > > > > > > timeout issue as we are not getting Rx interrupt even though
> > > > > > > SPRF bit is set in the status register.
> > > > > > >
> > > > > > > But there is no issue if we don't switch between interrupt to
> > > > > > > DMA mode or vice versa.
> > > > > > >
> > > > > > > Performance comparison between interrupt and DMA mode on
> > > > > > > RZ/Five SMARC platform connected to a display module shows
> > > > > > > that performance and CPU utilization is much better with DMA
> > > > > > > mode compared to interrupt mode
> > > > > > > (1->65 fps) and (98->8%).
> > > > > > >
> > > > > > > This patch introduces a variable force_dma to avoid switching
> > > > > > > between DMA to interrupt mode for RZ platforms.
> > 
> > Why do you need a variable for that, if DMA is availble (you were able to
> > allocate channels) then use DMA, otherwise fall back to PIO..
> 
> I was using DMA. We are not getting rspi interrupts after the DMA to PIO switch
> because of [1]. ie, we are not clearing DMAR in DMA driver
> and interrupt requests to the interrupt controller are masked.
> 
> [1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220719150000.383722-2-biju.das.jz@bp.renesas.com/
> 
> > 
> > Or anything missing from context which I am not aware of?
> 
> After this discussion, I have posted [1] and [2] to fix this issue.
> 
> [2] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220719150000.383722-1-biju.das.jz@bp.renesas.com/

Thanks for the explanation Biju. But why do we need .force_dma flag? 

> 
> Cheers,
> Biju
> 
> 
> > 
> > > > > > >
> > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > >
> > > > > > Thanks for your patch!
> > > > > >
> > > > > > > @@ -1196,6 +1197,7 @@ static const struct spi_ops rspi_rz_ops = {
> > > > > > >         .flags =                SPI_CONTROLLER_MUST_RX |
> > > > > > SPI_CONTROLLER_MUST_TX,
> > > > > > >         .fifo_size =            8,      /* 8 for TX, 32 for RX */
> > > > > > >         .num_hw_ss =            1,
> > > > > > > +       .force_dma =            true,
> > > > > > >  };
> > > > > >
> > > > > > Do you know if this is needed on RZ/A series, too?
> > > > >
> > > > > I guess it is needed?? I may be wrong. I got a link from Chris
> > > > > [1]. As per this still We haven't found a solution. May be the
> > > > > priority is changed for this activity and no one looked after this.
> > > > >
> > > > > [1]
> > > >
> > > > Daniel said he found the issue, i.e. the dmac driver never resetting
> > DMARS?
> > >
> > > Currently DMARS set during prepare and it never cleared. So I added
> > > device_synchronize callback in DMA driver to clear the DMARS. RSPI
> > > client driver after synchronizing DMA callback, calls
> > dmaengine_synchronize which clears DMARS.
> > >
> > > With this DMA to interrupt mode switching is working fine.
> > >
> > > Cheers,
> > > Biju
> > >
> > 
> > --
> > ~Vinod

-- 
~Vinod

RE: [PATCH] spi: spi-rspi: Add force_dma variable to spi_ops

[Biju Das]

Hi Vinod,

> Subject: Re: [PATCH] spi: spi-rspi: Add force_dma variable to spi_ops
> 
> On 20-07-22, 05:13, Biju Das wrote:
> > Hi Vinod,
> >
> > > Subject: Re: [PATCH] spi: spi-rspi: Add force_dma variable to
> > > spi_ops
> > >
> > > On 19-07-22, 11:28, Biju Das wrote:
> > > > Hi Geert,
> > > >
> > > > +Vinod
> > > >
> > > > > Subject: Re: [PATCH] spi: spi-rspi: Add force_dma variable to
> > > > > spi_ops
> > > > >
> > > > > Hi Biju,
> > > > >
> > > > > On Tue, Jul 19, 2022 at 10:29 AM Biju Das
> > > > > <biju.das.jz@bp.renesas.com>
> > > > > wrote:
> > > > > > > Subject: Re: [PATCH] spi: spi-rspi: Add force_dma variable
> > > > > > > to spi_ops On Sat, Jul 16, 2022 at 5:39 PM Biju Das
> > > > > <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > On RZ/G2L SoCs switching from DMA to interrupt mode,
> > > > > > > > causes timeout issue as we are not getting Rx interrupt
> > > > > > > > even though SPRF bit is set in the status register.
> > > > > > > >
> > > > > > > > But there is no issue if we don't switch between interrupt
> > > > > > > > to DMA mode or vice versa.
> > > > > > > >
> > > > > > > > Performance comparison between interrupt and DMA mode on
> > > > > > > > RZ/Five SMARC platform connected to a display module shows
> > > > > > > > that performance and CPU utilization is much better with
> > > > > > > > DMA mode compared to interrupt mode
> > > > > > > > (1->65 fps) and (98->8%).
> > > > > > > >
> > > > > > > > This patch introduces a variable force_dma to avoid
> > > > > > > > switching between DMA to interrupt mode for RZ platforms.
> > >
> > > Why do you need a variable for that, if DMA is availble (you were
> > > able to allocate channels) then use DMA, otherwise fall back to PIO..
> >
> > I was using DMA. We are not getting rspi interrupts after the DMA to
> > PIO switch because of [1]. ie, we are not clearing DMAR in DMA driver
> > and interrupt requests to the interrupt controller are masked.
> >
> > [1]
> >
> > >
> > > Or anything missing from context which I am not aware of?
> >
> > After this discussion, I have posted [1] and [2] to fix this issue.
> >
> > [2]
> 
> Thanks for the explanation Biju. But why do we need .force_dma flag?

It is not required. This patch is not valid anymore.

Initially I met with an issue(PIO fallback does not work). So posted this patch to make all transfer DMA by using .force_dma flag.

Then Mark suggested that we should find the root cause. After that, Geert mentioned 
we are not clearing DMARS, that is the reason for interrupt miss.

During DMA prepare, we set RSPI DMARS and signal is set for DMA transfer request signal
and it masks rspi interrupts. When we do PIO mode, still DMARS is set, and we won't get rspi
interrupt.

The new patches which I posted[1] and [2] clears DMARS in dmaengine_synchronize() in dma callback
after synchronizing with wait_event and PIO fallback works as expected.

[1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220719150000.383722-1-biju.das.jz@bp.renesas.com/
[2] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220719150000.383722-2-biju.das.jz@bp.renesas.com/

Cheers,
Biju

Re: [PATCH] spi: spi-rspi: Add force_dma variable to spi_ops

[Vinod Koul]

On 20-07-22, 10:54, Biju Das wrote:
> Hi Vinod,
> 
> > Subject: Re: [PATCH] spi: spi-rspi: Add force_dma variable to spi_ops
> > 
> > On 20-07-22, 05:13, Biju Das wrote:
> > > Hi Vinod,
> > >
> > > > Subject: Re: [PATCH] spi: spi-rspi: Add force_dma variable to
> > > > spi_ops
> > > >
> > > > On 19-07-22, 11:28, Biju Das wrote:
> > > > > Hi Geert,
> > > > >
> > > > > +Vinod
> > > > >
> > > > > > Subject: Re: [PATCH] spi: spi-rspi: Add force_dma variable to
> > > > > > spi_ops
> > > > > >
> > > > > > Hi Biju,
> > > > > >
> > > > > > On Tue, Jul 19, 2022 at 10:29 AM Biju Das
> > > > > > <biju.das.jz@bp.renesas.com>
> > > > > > wrote:
> > > > > > > > Subject: Re: [PATCH] spi: spi-rspi: Add force_dma variable
> > > > > > > > to spi_ops On Sat, Jul 16, 2022 at 5:39 PM Biju Das
> > > > > > <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > On RZ/G2L SoCs switching from DMA to interrupt mode,
> > > > > > > > > causes timeout issue as we are not getting Rx interrupt
> > > > > > > > > even though SPRF bit is set in the status register.
> > > > > > > > >
> > > > > > > > > But there is no issue if we don't switch between interrupt
> > > > > > > > > to DMA mode or vice versa.
> > > > > > > > >
> > > > > > > > > Performance comparison between interrupt and DMA mode on
> > > > > > > > > RZ/Five SMARC platform connected to a display module shows
> > > > > > > > > that performance and CPU utilization is much better with
> > > > > > > > > DMA mode compared to interrupt mode
> > > > > > > > > (1->65 fps) and (98->8%).
> > > > > > > > >
> > > > > > > > > This patch introduces a variable force_dma to avoid
> > > > > > > > > switching between DMA to interrupt mode for RZ platforms.
> > > >
> > > > Why do you need a variable for that, if DMA is availble (you were
> > > > able to allocate channels) then use DMA, otherwise fall back to PIO..
> > >
> > > I was using DMA. We are not getting rspi interrupts after the DMA to
> > > PIO switch because of [1]. ie, we are not clearing DMAR in DMA driver
> > > and interrupt requests to the interrupt controller are masked.
> > >
> > > [1]
> > >
> > > >
> > > > Or anything missing from context which I am not aware of?
> > >
> > > After this discussion, I have posted [1] and [2] to fix this issue.
> > >
> > > [2]
> > 
> > Thanks for the explanation Biju. But why do we need .force_dma flag?
> 
> It is not required. This patch is not valid anymore.

okay, that sounds about right!

> Initially I met with an issue(PIO fallback does not work). So posted this patch to make all transfer DMA by using .force_dma flag.
> 
> Then Mark suggested that we should find the root cause. After that, Geert mentioned 
> we are not clearing DMARS, that is the reason for interrupt miss.
> 
> During DMA prepare, we set RSPI DMARS and signal is set for DMA transfer request signal
> and it masks rspi interrupts. When we do PIO mode, still DMARS is set, and we won't get rspi
> interrupt.
> 
> The new patches which I posted[1] and [2] clears DMARS in dmaengine_synchronize() in dma callback
> after synchronizing with wait_event and PIO fallback works as expected.
> 
> [1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220719150000.383722-1-biju.das.jz@bp.renesas.com/
> [2] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220719150000.383722-2-biju.das.jz@bp.renesas.com/
> 
> Cheers,
> Biju

-- 
~Vinod