[v5,11/15] dmaengine: imx-sdma: fix ecspi1 rx dma not work on i.mx8mm

Commit Message

Robin Gong June 10, 2019, 8:17 a.m. UTC

From: Robin Gong <[e-mail]>

Because the number of ecspi1 rx event on i.mx8mm is 0, the condition
check ignore such special case without dma channel enabled, which caused
ecspi1 rx works failed. Actually, no need to check event_id0/event_id1
and replace checking 'event_id1' with 'DMA_DEV_TO_DEV', so that configure
event_id1 only in case DEV_TO_DEV.

Signed-off-by: Robin Gong <[e-mail]>
Acked-by: Vinod Koul <[e-mail]>
---
 drivers/dma/imx-sdma.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

Comments

Frieder Schrempf Sept. 25, 2019, 2:53 p.m. UTC | #1

On 25.09.19 13:26, Robin Gong wrote:
> On 2019-9-24 21:28 Schrempf Frieder <[e-mail]> wrote:
>>
>> Hi Robin,
>>
>>> From: Robin Gong <yibin.gong at nxp.com>
>>>
>>> Because the number of ecspi1 rx event on i.mx8mm is 0, the condition
>>> check ignore such special case without dma channel enabled, which
>>> caused
>>> ecspi1 rx works failed. Actually, no need to check event_id0/event_id1
>>> and replace checking 'event_id1' with 'DMA_DEV_TO_DEV', so that
>>> configure
>>> event_id1 only in case DEV_TO_DEV.
>>>
>>> Signed-off-by: Robin Gong <yibin.gong at nxp.com>
>>> Acked-by: Vinod Koul <vkoul at kernel.org>
>>
>> I have a custom board with i.MX8MM and SPI flash on ecspi1. I'm currently
>> testing with v5.3 and as SPI didn't work, I tried two different things:
>>
>> 1. Removing 'dmas' and 'dma-names' from the ecspi1 node in imx8mm.dtsi,
>>      to use PIO instead of DMA. This works as expected and the driver
>>      boots with the following messages:
>>
>>          spi_imx 30820000.spi: dma setup error -19, use pio
>>          m25p80 spi0.0: mx25v8035f (1024 Kbytes)
>>          spi_imx 30820000.spi: probed
>>
>> 2. Applying your patchset and use DMA. In this case, the flash also
>>      works fine, but there are some error messages printed while booting:
>>
>>          spi_master spi0: I/O Error in DMA RX
>>          m25p80 spi0.0: SPI transfer failed: -110
>>          spi_master spi0: failed to transfer one message from queue
>>          m25p80 spi0.0: mx25v8035f (1024 Kbytes)
>>          spi_imx 30820000.spi: probed
>>
>> It would be great to get your patches merged and fix SPI + DMA, but for
>> i.MX8MM, we need to get rid of the error messages. Do you have an idea,
>> what's wrong?

> Could you check if the length of spi message is bigger than fifo_size during
> spi_nor probe? If yes, at that time maybe sdma firmware not loaded.
> if (transfer->len < spi_imx->devtype_data->fifo_size)

Indeed, most of the transfers triggered by the SPI NOR dirver are below 
fifo_size and work fine, but some are bigger. The transfers therefore 
try to use DMA, but the firmware is not loaded yet.

How is this supposed to work? Shouldn't all transfers use PIO as long as 
the SDMA firmware is not loaded yet?

(+ Cc: [e-mail])

Robin Gong Sept. 27, 2019, 1:55 a.m. UTC | #2

On 2019-9-25 22:53 Schrempf Frieder <[e-mail]> wrote:
> On 25.09.19 13:26, Robin Gong wrote:
> > On 2019-9-24 21:28 Schrempf Frieder <[e-mail]>
> wrote:
> >>
> >> Hi Robin,
> >>
> >>> From: Robin Gong <yibin.gong at nxp.com>
> >>>
> >>> Because the number of ecspi1 rx event on i.mx8mm is 0, the condition
> >>> check ignore such special case without dma channel enabled, which
> >>> caused
> >>> ecspi1 rx works failed. Actually, no need to check
> >>> event_id0/event_id1 and replace checking 'event_id1' with
> >>> 'DMA_DEV_TO_DEV', so that configure
> >>> event_id1 only in case DEV_TO_DEV.
> >>>
> >>> Signed-off-by: Robin Gong <yibin.gong at nxp.com>
> >>> Acked-by: Vinod Koul <vkoul at kernel.org>
> >>
> >> I have a custom board with i.MX8MM and SPI flash on ecspi1. I'm
> >> currently testing with v5.3 and as SPI didn't work, I tried two different
> things:
> >>
> >> 1. Removing 'dmas' and 'dma-names' from the ecspi1 node in
> imx8mm.dtsi,
> >>      to use PIO instead of DMA. This works as expected and the driver
> >>      boots with the following messages:
> >>
> >>          spi_imx 30820000.spi: dma setup error -19, use pio
> >>          m25p80 spi0.0: mx25v8035f (1024 Kbytes)
> >>          spi_imx 30820000.spi: probed
> >>
> >> 2. Applying your patchset and use DMA. In this case, the flash also
> >>      works fine, but there are some error messages printed while
> booting:
> >>
> >>          spi_master spi0: I/O Error in DMA RX
> >>          m25p80 spi0.0: SPI transfer failed: -110
> >>          spi_master spi0: failed to transfer one message from queue
> >>          m25p80 spi0.0: mx25v8035f (1024 Kbytes)
> >>          spi_imx 30820000.spi: probed
> >>
> >> It would be great to get your patches merged and fix SPI + DMA, but
> >> for i.MX8MM, we need to get rid of the error messages. Do you have an
> >> idea, what's wrong?
> 
> > Could you check if the length of spi message is bigger than fifo_size
> > during spi_nor probe? If yes, at that time maybe sdma firmware not loaded.
> > if (transfer->len < spi_imx->devtype_data->fifo_size)
> 
> Indeed, most of the transfers triggered by the SPI NOR dirver are below
> fifo_size and work fine, but some are bigger. The transfers therefore try to
> use DMA, but the firmware is not loaded yet.
> 
> How is this supposed to work? Shouldn't all transfers use PIO as long as the
> SDMA firmware is not loaded yet?
Yes, for ecspi should work with ram script, it's better check if sdma firmware
is ready in spi_imx_dma_configure(), need modification in sdma driver too.
I'll create another patch after this patch set accepted. 
> 
> (+ Cc: [e-mail])

Frieder Schrempf Feb. 6, 2020, 10:17 a.m. UTC | #3

Hi,

On 27.09.19 03:55, Robin Gong wrote:
> On 2019-9-25 22:53 Schrempf Frieder <[e-mail]> wrote:
>> On 25.09.19 13:26, Robin Gong wrote:
>>> On 2019-9-24 21:28 Schrempf Frieder <[e-mail]>
>> wrote:
>>>>
>>>> Hi Robin,
>>>>
>>>>> From: Robin Gong <yibin.gong at nxp.com>
>>>>>
>>>>> Because the number of ecspi1 rx event on i.mx8mm is 0, the condition
>>>>> check ignore such special case without dma channel enabled, which
>>>>> caused
>>>>> ecspi1 rx works failed. Actually, no need to check
>>>>> event_id0/event_id1 and replace checking 'event_id1' with
>>>>> 'DMA_DEV_TO_DEV', so that configure
>>>>> event_id1 only in case DEV_TO_DEV.
>>>>>
>>>>> Signed-off-by: Robin Gong <yibin.gong at nxp.com>
>>>>> Acked-by: Vinod Koul <vkoul at kernel.org>
>>>>
>>>> I have a custom board with i.MX8MM and SPI flash on ecspi1. I'm
>>>> currently testing with v5.3 and as SPI didn't work, I tried two different
>> things:
>>>>
>>>> 1. Removing 'dmas' and 'dma-names' from the ecspi1 node in
>> imx8mm.dtsi,
>>>>       to use PIO instead of DMA. This works as expected and the driver
>>>>       boots with the following messages:
>>>>
>>>>           spi_imx 30820000.spi: dma setup error -19, use pio
>>>>           m25p80 spi0.0: mx25v8035f (1024 Kbytes)
>>>>           spi_imx 30820000.spi: probed
>>>>
>>>> 2. Applying your patchset and use DMA. In this case, the flash also
>>>>       works fine, but there are some error messages printed while
>> booting:
>>>>
>>>>           spi_master spi0: I/O Error in DMA RX
>>>>           m25p80 spi0.0: SPI transfer failed: -110
>>>>           spi_master spi0: failed to transfer one message from queue
>>>>           m25p80 spi0.0: mx25v8035f (1024 Kbytes)
>>>>           spi_imx 30820000.spi: probed
>>>>
>>>> It would be great to get your patches merged and fix SPI + DMA, but
>>>> for i.MX8MM, we need to get rid of the error messages. Do you have an
>>>> idea, what's wrong?
>>
>>> Could you check if the length of spi message is bigger than fifo_size
>>> during spi_nor probe? If yes, at that time maybe sdma firmware not loaded.
>>> if (transfer->len < spi_imx->devtype_data->fifo_size)
>>
>> Indeed, most of the transfers triggered by the SPI NOR dirver are below
>> fifo_size and work fine, but some are bigger. The transfers therefore try to
>> use DMA, but the firmware is not loaded yet.
>>
>> How is this supposed to work? Shouldn't all transfers use PIO as long as the
>> SDMA firmware is not loaded yet?
> Yes, for ecspi should work with ram script, it's better check if sdma firmware
> is ready in spi_imx_dma_configure(), need modification in sdma driver too.
> I'll create another patch after this patch set accepted.

This still seems to be broken upstream. Is anyone working on fixing SPI 
+ DMA for i.MX8MM? Otherwise I will send a patch that removes the DMA 
from the ecspi nodes in the devicetree.

Thanks,
Frieder

Robin Gong Feb. 10, 2020, 12:28 p.m. UTC | #4

On 2020/02/06 Schrempf Frieder <[e-mail]> wrote:
> 
> Hi,
> On 27.09.19 03:55, Robin Gong wrote:
> > On 2019-9-25 22:53 Schrempf Frieder <[e-mail]> wrote:
> >> On 25.09.19 13:26, Robin Gong wrote:
> >>> On 2019-9-24 21:28 Schrempf Frieder <[e-mail]>
> >> wrote:
> >>>>
> >>>> Hi Robin,
> >>>>
> >>>>> From: Robin Gong <yibin.gong at nxp.com>
> >>>>>
> >>>>> Because the number of ecspi1 rx event on i.mx8mm is 0, the
> >>>>> condition check ignore such special case without dma channel
> >>>>> enabled, which caused
> >>>>> ecspi1 rx works failed. Actually, no need to check
> >>>>> event_id0/event_id1 and replace checking 'event_id1' with
> >>>>> 'DMA_DEV_TO_DEV', so that configure
> >>>>> event_id1 only in case DEV_TO_DEV.
> >>>>>
> >>>>> Signed-off-by: Robin Gong <yibin.gong at nxp.com>
> >>>>> Acked-by: Vinod Koul <vkoul at kernel.org>
> >>>>
> >>>> I have a custom board with i.MX8MM and SPI flash on ecspi1. I'm
> >>>> currently testing with v5.3 and as SPI didn't work, I tried two
> >>>> different
> >> things:
> >>>>
> >>>> 1. Removing 'dmas' and 'dma-names' from the ecspi1 node in
> >> imx8mm.dtsi,
> >>>>       to use PIO instead of DMA. This works as expected and the driver
> >>>>       boots with the following messages:
> >>>>
> >>>>           spi_imx 30820000.spi: dma setup error -19, use pio
> >>>>           m25p80 spi0.0: mx25v8035f (1024 Kbytes)
> >>>>           spi_imx 30820000.spi: probed
> >>>>
> >>>> 2. Applying your patchset and use DMA. In this case, the flash also
> >>>>       works fine, but there are some error messages printed while
> >> booting:
> >>>>
> >>>>           spi_master spi0: I/O Error in DMA RX
> >>>>           m25p80 spi0.0: SPI transfer failed: -110
> >>>>           spi_master spi0: failed to transfer one message from queue
> >>>>           m25p80 spi0.0: mx25v8035f (1024 Kbytes)
> >>>>           spi_imx 30820000.spi: probed
> >>>>
> >>>> It would be great to get your patches merged and fix SPI + DMA, but
> >>>> for i.MX8MM, we need to get rid of the error messages. Do you have
> >>>> an idea, what's wrong?
> >>
> >>> Could you check if the length of spi message is bigger than
> >>> fifo_size during spi_nor probe? If yes, at that time maybe sdma firmware
> not loaded.
> >>> if (transfer->len < spi_imx->devtype_data->fifo_size)
> >>
> >> Indeed, most of the transfers triggered by the SPI NOR dirver are
> >> below fifo_size and work fine, but some are bigger. The transfers
> >> therefore try to use DMA, but the firmware is not loaded yet.
> >>
> >> How is this supposed to work? Shouldn't all transfers use PIO as long
> >> as the SDMA firmware is not loaded yet?
> > Yes, for ecspi should work with ram script, it's better check if sdma
> > firmware is ready in spi_imx_dma_configure(), need modification in sdma
> driver too.
> > I'll create another patch after this patch set accepted.
> 
> This still seems to be broken upstream. Is anyone working on fixing SPI
> + DMA for i.MX8MM? Otherwise I will send a patch that removes the DMA
> from the ecspi nodes in the devicetree.
Sorry, Frieder, my patch set has been blocked for so long time because I revert
some patches and need get commit from patch owner, unfortunately, I didn't
get any feedback from them although I got ACK from maintainer. I'll pick up
them, rebase and resend it these days, then could address your issue later.

Patch

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 525ca89..f7c150d 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -1202,7 +1202,7 @@  static int sdma_config_channel(struct dma_chan *chan)
 	if ((sdmac->peripheral_type != IMX_DMATYPE_MEMORY) &&
 			(sdmac->peripheral_type != IMX_DMATYPE_DSP)) {
 		/* Handle multiple event channels differently */
-		if (sdmac->event_id1) {
+		if (sdmac->direction == DMA_DEV_TO_DEV) {
 			if (sdmac->peripheral_type == IMX_DMATYPE_ASRC_SP ||
 			    sdmac->peripheral_type == IMX_DMATYPE_ASRC)
 				sdma_set_watermarklevel_for_p2p(sdmac);
@@ -1370,9 +1370,9 @@  static void sdma_free_chan_resources(struct dma_chan *chan)
 
 	sdma_channel_synchronize(chan);
 
-	if (sdmac->event_id0)
-		sdma_event_disable(sdmac, sdmac->event_id0);
-	if (sdmac->event_id1)
+	sdma_event_disable(sdmac, sdmac->event_id0);
+
+	if (sdmac->direction == DMA_DEV_TO_DEV)
 		sdma_event_disable(sdmac, sdmac->event_id1);
 
 	sdmac->event_id0 = 0;
@@ -1670,13 +1670,11 @@  static int sdma_config(struct dma_chan *chan,
 	memcpy(&sdmac->slave_config, dmaengine_cfg, sizeof(*dmaengine_cfg));
 
 	/* Set ENBLn earlier to make sure dma request triggered after that */
-	if (sdmac->event_id0) {
-		if (sdmac->event_id0 >= sdmac->sdma->drvdata->num_events)
-			return -EINVAL;
-		sdma_event_enable(sdmac, sdmac->event_id0);
-	}
+	if (sdmac->event_id0 >= sdmac->sdma->drvdata->num_events)
+		return -EINVAL;
+	sdma_event_enable(sdmac, sdmac->event_id0);
 
-	if (sdmac->event_id1) {
+	if (sdmac->direction == DMA_DEV_TO_DEV) {
 		if (sdmac->event_id1 >= sdmac->sdma->drvdata->num_events)
 			return -EINVAL;
 		sdma_event_enable(sdmac, sdmac->event_id1);