| Message ID | 20220716153934.292311-1-biju.das.jz@bp.renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | spi: spi-rspi: Add force_dma variable to spi_ops | expand |
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
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 >
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
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
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
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
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 = {
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(-)