[PATCH] spi: imx: Don't expect DMA for i.MX{25,35,50,51,53} cspi devices

[PATCH] spi: imx: Don't expect DMA for i.MX{25,35,50,51,53} cspi devices

[Uwe Kleine-König]

While in commit 2dd33f9cec90 ("spi: imx: support DMA for imx35") it was
claimed that DMA works on i.MX25, i.MX31 and i.MX35 the respective
device trees don't add DMA channels. The Reference manuals of i.MX31 and
i.MX25 also don't mention the CSPI core being DMA capable. (I didn't
check the others.)

Since commit e267a5b3ec59 ("spi: spi-imx: Use dev_err_probe for failed
DMA channel requests") this results in an error message

	spi_imx 43fa4000.spi: error -ENODEV: can't get the TX DMA channel!

during boot. However that isn't fatal and the driver gets loaded just
fine, just without using DMA.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/spi/spi-imx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index f4006c82f867..4de5476f79b8 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -1060,7 +1060,7 @@ static struct spi_imx_devtype_data imx35_cspi_devtype_data = {
 	.rx_available = mx31_rx_available,
 	.reset = mx31_reset,
 	.fifo_size = 8,
-	.has_dmamode = true,
+	.has_dmamode = false,
 	.dynamic_burst = false,
 	.has_targetmode = false,
 	.devtype = IMX35_CSPI,

base-commit: e7b4ef8fffaca247809337bb78daceb406659f2d
-- 
2.43.0

Re: [PATCH] spi: imx: Don't expect DMA for i.MX{25,35,50,51,53} cspi devices

[Martin Kaiser]

Hi Uwe,

Thus wrote Uwe Kleine-König (u.kleine-koenig@pengutronix.de):

> While in commit 2dd33f9cec90 ("spi: imx: support DMA for imx35") it was
> claimed that DMA works on i.MX25, i.MX31 and i.MX35 the respective
> device trees don't add DMA channels. The Reference manuals of i.MX31 and
> i.MX25 also don't mention the CSPI core being DMA capable. (I didn't
> check the others.)

If I'm not mistaken, the imx25 reference manual

https://www.nxp.com/docs/en/reference-manual/IMX25RM.pdf

does say that CSPI has DMA support. Section 18.1.1 (Features) lists DMA as one
of the features. There's also DMA events (section 3) for CSPI-1/2/3 RX, TX.

I'd have to dig up the hardware to test again if this is actually working...

   Martin

Re: [PATCH] spi: imx: Don't expect DMA for i.MX{25, 35, 50, 51, 53} cspi devices

[Uwe Kleine-König]

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

Hello Martin,

On Wed, May 08, 2024 at 03:44:53PM +0200, Martin Kaiser wrote:
> Thus wrote Uwe Kleine-König (u.kleine-koenig@pengutronix.de):
> > While in commit 2dd33f9cec90 ("spi: imx: support DMA for imx35") it was
> > claimed that DMA works on i.MX25, i.MX31 and i.MX35 the respective
> > device trees don't add DMA channels. The Reference manuals of i.MX31 and
> > i.MX25 also don't mention the CSPI core being DMA capable. (I didn't
> > check the others.)
> 
> If I'm not mistaken, the imx25 reference manual
> 
> https://www.nxp.com/docs/en/reference-manual/IMX25RM.pdf
> 
> does say that CSPI has DMA support. Section 18.1.1 (Features) lists DMA as one
> of the features. There's also DMA events (section 3) for CSPI-1/2/3 RX, TX.

Oh indeed. I don't know what made me claim that DMA isn't mentioned in
the reference manual. Maybe I looked at the i2c chapter.

I now did:

diff --git a/arch/arm/boot/dts/nxp/imx/imx25.dtsi b/arch/arm/boot/dts/nxp/imx/imx25.dtsi
index 4a85684deff8..710b28a41bae 100644
--- a/arch/arm/boot/dts/nxp/imx/imx25.dtsi
+++ b/arch/arm/boot/dts/nxp/imx/imx25.dtsi
@@ -190,6 +190,8 @@ spi1: spi@43fa4000 {
 				reg = <0x43fa4000 0x4000>;
 				clocks = <&clks 78>, <&clks 78>;
 				clock-names = "ipg", "per";
+				dmas = <&sdma 8 1 0>, <&sdma 9 1 0>;
+				dma-names = "rx", "tx";
 				interrupts = <14>;
 				status = "disabled";
 			};
@@ -229,6 +231,8 @@ spi3: spi@50004000 {
 				interrupts = <0>;
 				clocks = <&clks 80>, <&clks 80>;
 				clock-names = "ipg", "per";
+				dmas = <&sdma 34 1 0>, <&sdma 35 1 0>;
+				dma-names = "rx", "tx";
 				status = "disabled";
 			};
 
@@ -257,6 +261,8 @@ spi2: spi@50010000 {
 				reg = <0x50010000 0x4000>;
 				clocks = <&clks 79>, <&clks 79>;
 				clock-names = "ipg", "per";
+				dmas = <&sdma 6 1 0>, <&sdma 7 1 0>;
+				dma-names = "rx", "tx";
 				interrupts = <13>;
 				status = "disabled";
 			};

And spi still works. I see an issue, but I think that's orthogonal to
adding DMA. Will send a formal patch when I debugged that.

Thanks for your feedback,
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH] spi: imx: Don't expect DMA for i.MX{25, 35, 50, 51, 53} cspi devices

[Uwe Kleine-König]

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

Hello,

On Fri, May 10, 2024 at 02:32:56PM +0200, Uwe Kleine-König wrote:
> On Wed, May 08, 2024 at 03:44:53PM +0200, Martin Kaiser wrote:
> > Thus wrote Uwe Kleine-König (u.kleine-koenig@pengutronix.de):
> > > While in commit 2dd33f9cec90 ("spi: imx: support DMA for imx35") it was
> > > claimed that DMA works on i.MX25, i.MX31 and i.MX35 the respective
> > > device trees don't add DMA channels. The Reference manuals of i.MX31 and
> > > i.MX25 also don't mention the CSPI core being DMA capable. (I didn't
> > > check the others.)
> > 
> > If I'm not mistaken, the imx25 reference manual
> > 
> > https://www.nxp.com/docs/en/reference-manual/IMX25RM.pdf
> > 
> > does say that CSPI has DMA support. Section 18.1.1 (Features) lists DMA as one
> > of the features. There's also DMA events (section 3) for CSPI-1/2/3 RX, TX.
> 
> Oh indeed. I don't know what made me claim that DMA isn't mentioned in
> the reference manual. Maybe I looked at the i2c chapter.
> 
> I now did:
> 
> diff --git a/arch/arm/boot/dts/nxp/imx/imx25.dtsi b/arch/arm/boot/dts/nxp/imx/imx25.dtsi
> index 4a85684deff8..710b28a41bae 100644
> --- a/arch/arm/boot/dts/nxp/imx/imx25.dtsi
> +++ b/arch/arm/boot/dts/nxp/imx/imx25.dtsi
> @@ -190,6 +190,8 @@ spi1: spi@43fa4000 {
>  				reg = <0x43fa4000 0x4000>;
>  				clocks = <&clks 78>, <&clks 78>;
>  				clock-names = "ipg", "per";
> +				dmas = <&sdma 8 1 0>, <&sdma 9 1 0>;
> +				dma-names = "rx", "tx";
>  				interrupts = <14>;
>  				status = "disabled";
>  			};
> @@ -229,6 +231,8 @@ spi3: spi@50004000 {
>  				interrupts = <0>;
>  				clocks = <&clks 80>, <&clks 80>;
>  				clock-names = "ipg", "per";
> +				dmas = <&sdma 34 1 0>, <&sdma 35 1 0>;
> +				dma-names = "rx", "tx";
>  				status = "disabled";
>  			};
>  
> @@ -257,6 +261,8 @@ spi2: spi@50010000 {
>  				reg = <0x50010000 0x4000>;
>  				clocks = <&clks 79>, <&clks 79>;
>  				clock-names = "ipg", "per";
> +				dmas = <&sdma 6 1 0>, <&sdma 7 1 0>;
> +				dma-names = "rx", "tx";
>  				interrupts = <13>;
>  				status = "disabled";
>  			};

Additionally to the above I now did:

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index c3e5cee18bea..74da1a965a0d 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -1428,12 +1428,9 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
 	if (ret)
 		goto dma_failure_no_start;
 
-	if (!spi_imx->devtype_data->setup_wml) {
-		dev_err(spi_imx->dev, "No setup_wml()?\n");
-		ret = -EINVAL;
-		goto dma_failure_no_start;
+	if (spi_imx->devtype_data->setup_wml) {
+		spi_imx->devtype_data->setup_wml(spi_imx);
 	}
-	spi_imx->devtype_data->setup_wml(spi_imx);
 
 	/*
 	 * The TX DMA setup starts the transfer, so make sure RX is configured


because there is no .setup_wml() callback for i.MX25 in
imx31_cspi_devtype_data and the DMA register is already setup in
mx31_prepare_transfer(). Without this change DMA isn't used.

However this breaks SPI transfers, when I try to read out an MRAM I get:

	root@ecu02:~ hexdump -C /dev/mtd4
	[   71.813807] spi_imx 43fa4000.spi: I/O Error in DMA TX
	[   71.819173] spi-nor spi0.2: SPI transfer failed: -110
	[   71.829129] spi_master spi0: failed to transfer one message from queue
	[   71.843962] spi_master spi0: noqueue transfer failed

So it would indeed be interesting if you ever managed to use DMA on
i.MX25.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH] spi: imx: Don't expect DMA for i.MX{25, 35, 50, 51, 53} cspi devices

[Martin Kaiser]

Hello,

Thus wrote Uwe Kleine-König (u.kleine-koenig@pengutronix.de):

> Hello,

> On Fri, May 10, 2024 at 02:32:56PM +0200, Uwe Kleine-König wrote:
> > On Wed, May 08, 2024 at 03:44:53PM +0200, Martin Kaiser wrote:
> > > Thus wrote Uwe Kleine-König (u.kleine-koenig@pengutronix.de):
> > > > While in commit 2dd33f9cec90 ("spi: imx: support DMA for imx35") it was
> > > > claimed that DMA works on i.MX25, i.MX31 and i.MX35 the respective
> > > > device trees don't add DMA channels. The Reference manuals of i.MX31 and
> > > > i.MX25 also don't mention the CSPI core being DMA capable. (I didn't
> > > > check the others.)

> > > If I'm not mistaken, the imx25 reference manual

> > > https://www.nxp.com/docs/en/reference-manual/IMX25RM.pdf

> > > does say that CSPI has DMA support. Section 18.1.1 (Features) lists DMA as one
> > > of the features. There's also DMA events (section 3) for CSPI-1/2/3 RX, TX.

> > Oh indeed. I don't know what made me claim that DMA isn't mentioned in
> > the reference manual. Maybe I looked at the i2c chapter.

> > I now did:

> > diff --git a/arch/arm/boot/dts/nxp/imx/imx25.dtsi b/arch/arm/boot/dts/nxp/imx/imx25.dtsi
> > index 4a85684deff8..710b28a41bae 100644
> > --- a/arch/arm/boot/dts/nxp/imx/imx25.dtsi
> > +++ b/arch/arm/boot/dts/nxp/imx/imx25.dtsi
> > @@ -190,6 +190,8 @@ spi1: spi@43fa4000 {
> >  				reg = <0x43fa4000 0x4000>;
> >  				clocks = <&clks 78>, <&clks 78>;
> >  				clock-names = "ipg", "per";
> > +				dmas = <&sdma 8 1 0>, <&sdma 9 1 0>;
> > +				dma-names = "rx", "tx";
> >  				interrupts = <14>;
> >  				status = "disabled";
> >  			};
> > @@ -229,6 +231,8 @@ spi3: spi@50004000 {
> >  				interrupts = <0>;
> >  				clocks = <&clks 80>, <&clks 80>;
> >  				clock-names = "ipg", "per";
> > +				dmas = <&sdma 34 1 0>, <&sdma 35 1 0>;
> > +				dma-names = "rx", "tx";
> >  				status = "disabled";
> >  			};

> > @@ -257,6 +261,8 @@ spi2: spi@50010000 {
> >  				reg = <0x50010000 0x4000>;
> >  				clocks = <&clks 79>, <&clks 79>;
> >  				clock-names = "ipg", "per";
> > +				dmas = <&sdma 6 1 0>, <&sdma 7 1 0>;
> > +				dma-names = "rx", "tx";
> >  				interrupts = <13>;
> >  				status = "disabled";
> >  			};

> Additionally to the above I now did:

> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index c3e5cee18bea..74da1a965a0d 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -1428,12 +1428,9 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
>  	if (ret)
>  		goto dma_failure_no_start;

> -	if (!spi_imx->devtype_data->setup_wml) {
> -		dev_err(spi_imx->dev, "No setup_wml()?\n");
> -		ret = -EINVAL;
> -		goto dma_failure_no_start;
> +	if (spi_imx->devtype_data->setup_wml) {
> +		spi_imx->devtype_data->setup_wml(spi_imx);
>  	}
> -	spi_imx->devtype_data->setup_wml(spi_imx);

>  	/*
>  	 * The TX DMA setup starts the transfer, so make sure RX is configured


> because there is no .setup_wml() callback for i.MX25 in
> imx31_cspi_devtype_data and the DMA register is already setup in
> mx31_prepare_transfer(). Without this change DMA isn't used.

> However this breaks SPI transfers, when I try to read out an MRAM I get:

> 	root@ecu02:~ hexdump -C /dev/mtd4
> 	[   71.813807] spi_imx 43fa4000.spi: I/O Error in DMA TX
> 	[   71.819173] spi-nor spi0.2: SPI transfer failed: -110
> 	[   71.829129] spi_master spi0: failed to transfer one message from queue
> 	[   71.843962] spi_master spi0: noqueue transfer failed

> So it would indeed be interesting if you ever managed to use DMA on
> i.MX25.

I believe so. Looking into my notes from 2016 (when I last tried this), I got
the -110 error when the SMC bit in CONREG wasn't set. But this should now be
done in mx31_prepare_transfer if DMA is used...

I'll try to set up a test some time next week.

Best regards,
Martin

Re: [PATCH] spi: imx: Don't expect DMA for i.MX{25, 35, 50, 51, 53} cspi devices

[Martin Kaiser]

Thus wrote Martin Kaiser (martin@kaiser.cx):

> > However this breaks SPI transfers, when I try to read out an MRAM I get:

> > 	root@ecu02:~ hexdump -C /dev/mtd4
> > 	[   71.813807] spi_imx 43fa4000.spi: I/O Error in DMA TX
> > 	[   71.819173] spi-nor spi0.2: SPI transfer failed: -110
> > 	[   71.829129] spi_master spi0: failed to transfer one message from queue
> > 	[   71.843962] spi_master spi0: noqueue transfer failed

> > So it would indeed be interesting if you ever managed to use DMA on
> > i.MX25.

> I believe so. Looking into my notes from 2016 (when I last tried this), I got
> the -110 error when the SMC bit in CONREG wasn't set. But this should now be
> done in mx31_prepare_transfer if DMA is used...

> I'll try to set up a test some time next week.

I can now reproduce the dma tx error. My test board uses a dummy spidev device
on cspi1 and configures loopback on the cspi1 controller (disabling loopback
makes no difference).

I'm relatively sure that this setup did work back in 2016/17.

Commit 24bb244e02a6 ("ARM: i.MX25: globally disable supervisor protect") was
required before any DMA transfers started working on imx25. As far as I can
see, this code is still present.

I'll look into this a bit more as time permits.

   Martin

Re: [PATCH] spi: imx: Don't expect DMA for i.MX{25, 35, 50, 51, 53} cspi devices

[Marc Kleine-Budde]

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

Hello Martin,

On 15.05.2024 21:26:17, Martin Kaiser wrote:
> Thus wrote Martin Kaiser (martin@kaiser.cx):
> 
> > > However this breaks SPI transfers, when I try to read out an MRAM I get:
> 
> > > 	root@ecu02:~ hexdump -C /dev/mtd4
> > > 	[   71.813807] spi_imx 43fa4000.spi: I/O Error in DMA TX
> > > 	[   71.819173] spi-nor spi0.2: SPI transfer failed: -110
> > > 	[   71.829129] spi_master spi0: failed to transfer one message from queue
> > > 	[   71.843962] spi_master spi0: noqueue transfer failed
> 
> > > So it would indeed be interesting if you ever managed to use DMA on
> > > i.MX25.
> 
> > I believe so. Looking into my notes from 2016 (when I last tried this), I got
> > the -110 error when the SMC bit in CONREG wasn't set. But this should now be
> > done in mx31_prepare_transfer if DMA is used...
> 
> > I'll try to set up a test some time next week.
> 
> I can now reproduce the dma tx error. My test board uses a dummy spidev device
> on cspi1 and configures loopback on the cspi1 controller (disabling loopback
> makes no difference).
> 
> I'm relatively sure that this setup did work back in 2016/17.
> 
> Commit 24bb244e02a6 ("ARM: i.MX25: globally disable supervisor protect") was
> required before any DMA transfers started working on imx25. As far as I can
> see, this code is still present.
> 
> I'll look into this a bit more as time permits.

have you found the time to look into this?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Re: [PATCH] spi: imx: Don't expect DMA for i.MX{25, 35, 50, 51, 53} cspi devices

[Martin Kaiser]

Hello Marc,

Thus wrote Marc Kleine-Budde (mkl@pengutronix.de):

> Hello Martin,

> On 15.05.2024 21:26:17, Martin Kaiser wrote:
> > Thus wrote Martin Kaiser (martin@kaiser.cx):

> > > > However this breaks SPI transfers, when I try to read out an MRAM I get:

> > > > 	root@ecu02:~ hexdump -C /dev/mtd4
> > > > 	[   71.813807] spi_imx 43fa4000.spi: I/O Error in DMA TX
> > > > 	[   71.819173] spi-nor spi0.2: SPI transfer failed: -110
> > > > 	[   71.829129] spi_master spi0: failed to transfer one message from queue
> > > > 	[   71.843962] spi_master spi0: noqueue transfer failed

> > > > So it would indeed be interesting if you ever managed to use DMA on
> > > > i.MX25.

> > > I believe so. Looking into my notes from 2016 (when I last tried this), I got
> > > the -110 error when the SMC bit in CONREG wasn't set. But this should now be
> > > done in mx31_prepare_transfer if DMA is used...

> > > I'll try to set up a test some time next week.

> > I can now reproduce the dma tx error. My test board uses a dummy spidev device
> > on cspi1 and configures loopback on the cspi1 controller (disabling loopback
> > makes no difference).

> > I'm relatively sure that this setup did work back in 2016/17.

> > Commit 24bb244e02a6 ("ARM: i.MX25: globally disable supervisor protect") was
> > required before any DMA transfers started working on imx25. As far as I can
> > see, this code is still present.

> > I'll look into this a bit more as time permits.

> have you found the time to look into this?

I did have another look but couldn't get DMA to work :-(

I got to the point where tx seemed to work and rx failed. The tx callback
function was called from the dma core and rx timed out. However, looking at
the mosi, miso and clk signals, there was no clock and no data transmission on
the bus.

The dev_dbg prints in imx-sdma didn't show anything that looked wrong...

I'll have another go some time soon. In the meantime, feel free to merge this
patch and disable DMA on imx25.

Best regards,
Martin

Re: [PATCH] spi: imx: Don't expect DMA for i.MX{25,35,50,51,53} cspi devices

[Mark Brown]

On Wed, 08 May 2024 11:56:10 +0200, Uwe Kleine-König wrote:
> While in commit 2dd33f9cec90 ("spi: imx: support DMA for imx35") it was
> claimed that DMA works on i.MX25, i.MX31 and i.MX35 the respective
> device trees don't add DMA channels. The Reference manuals of i.MX31 and
> i.MX25 also don't mention the CSPI core being DMA capable. (I didn't
> check the others.)
> 
> Since commit e267a5b3ec59 ("spi: spi-imx: Use dev_err_probe for failed
> DMA channel requests") this results in an error message
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: imx: Don't expect DMA for i.MX{25,35,50,51,53} cspi devices
      commit: 5d0c35feea339e4a3a9c9e99731e4d49ad5ee329

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark