OMAP: McSPI: Fix RX DMA transfer path

Commit Message

Koskinen, Aaro (Nokia - FI/Espoo) June 12, 2009, 12:31 p.m. UTC

From: Eero Nurkkala <[e-mail]>

When data is read through DMA, the last element must be read separately
through the RX register. It cannot be transferred by the DMA. For further
details see e.g. OMAP3430 TRM.

Without the fix the driver causes extra clocks to be clocked to the
bus after DMA RX operations. This can cause interesting behaviour with
some devices.

Reported-by: Juuso Oikarinen <[e-mail]>
Signed-off-by: Juuso Oikarinen <[e-mail]>
Signed-off-by: Eero Nurkkala <[e-mail]>
[[e-mail]: Simplified the patch while keeping the idea.]
Signed-off-by: Aaro Koskinen <[e-mail]>
---
 drivers/spi/omap2_mcspi.c |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)

Comments

Kalle Valo June 13, 2009, 4:37 p.m. UTC | #1

Aaro Koskinen <[e-mail]> writes:

> From: Eero Nurkkala <[e-mail]>
>
> When data is read through DMA, the last element must be read separately
> through the RX register. It cannot be transferred by the DMA. For further
> details see e.g. OMAP3430 TRM.
>
> Without the fix the driver causes extra clocks to be clocked to the
> bus after DMA RX operations. This can cause interesting behaviour with
> some devices.

I was hit by this with stlc45xx and earlier versions of Juuso's patch at
least fixed the problem.

I will test this patch as soon as I get N800 boot with latest
linux-omap, now I only get this:

<6>JFFS2 version 2.2. (NAND) (SUMMARY)  © 2001-2006 Red Hat, Inc.
<6>JFFS2 version 2.2. (NAND) (SUMMARY)  © 2001-2006 Red H
<6>msgmni has been set to 247
<6>io scheduler noop registered (default)
<6>omapfb: ls041y3 rev 87 LCD detected, 0 data lines      d
<6>serial8250.0: ttyS0 at MMIO 0x4806a000 (irq = 72) is a ST16654
<6>serial8250.0: ttyS1 at MMIO 0x4806e000 (irq = 74) is a ST16654
<ó"F:­5 #!*$Õ<ó¬#0­5 #!*$Õ<ó­r%
µ²² æ2Õ<ó$¶ #®*#5##¹'%Õ<ó#62°$¶ ¹)K!¥º¹'Õ<ó¬¹5 K#z 0«²)Ñ%f¡T:6æ0ªÿ<ó¯
ó
%°ö2­Õ³#²©T ô­#.#É ,­#2¬#£µ0V³/­5 V§Í#%Rí<ó00¢F ô- ¹##£$¶ +3Sí<ó
0
·#
  # V60±:)¥#
,#µ¥Õ46 V#Õ#
¬é)¶.¹ªÿ<óµ&£É%²:©F%V$®º ¹##£$¶
 Í#É¡*ªÿ<óµ&                            ©#­0Á:£µ­.#µ¥Õ4#Ô#Kªÿ#ó#Í!Ä)
 %
³3{%°ö2­±9±Sí<ó"Õ {/+#í#ó#Í!Ä)4?+#í#ó#Í!Ä)4ó ö£U4¬¹ ¹)K!)º"ªÿ<ó"Õ%{/C:³ô
Ù¯    s
®Ñ ®®Ò 335<#U2,°®!Ä)  ô2V#"ö/ë"¢#4/##®#%!V C%!

Hemanth V June 15, 2009, 5:28 a.m. UTC | #2

----- Original Message ----- 
From: "Aaro Koskinen" <[e-mail]>
Subject: [PATCH] OMAP: McSPI: Fix RX DMA transfer path


> From: Eero Nurkkala <[e-mail]>
>
> When data is read through DMA, the last element must be read separately
> through the RX register. It cannot be transferred by the DMA. For further
> details see e.g. OMAP3430 TRM.

Could you provide the section number and the version of TRM you are
referring to.

>
> Without the fix the driver causes extra clocks to be clocked to the
> bus after DMA RX operations. This can cause interesting behaviour with
> some devices.

Could you provide more details on the errors seen i.e is the transfer 
aborted

Thanks
Hemanth

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tony Lindgren June 15, 2009, 7:33 a.m. UTC | #3

* Kalle Valo <[e-mail]> [090613 09:45]:
> Aaro Koskinen <[e-mail]> writes:
> 
> > From: Eero Nurkkala <[e-mail]>
> >
> > When data is read through DMA, the last element must be read separately
> > through the RX register. It cannot be transferred by the DMA. For further
> > details see e.g. OMAP3430 TRM.
> >
> > Without the fix the driver causes extra clocks to be clocked to the
> > bus after DMA RX operations. This can cause interesting behaviour with
> > some devices.
> 
> I was hit by this with stlc45xx and earlier versions of Juuso's patch at
> least fixed the problem.
> 
> I will test this patch as soon as I get N800 boot with latest
> linux-omap, now I only get this:
> 
> <6>JFFS2 version 2.2. (NAND) (SUMMARY)  © 2001-2006 Red Hat, Inc.
> <6>JFFS2 version 2.2. (NAND) (SUMMARY)  © 2001-2006 Red H
> <6>msgmni has been set to 247
> <6>io scheduler noop registered (default)
> <6>omapfb: ls041y3 rev 87 LCD detected, 0 data lines      d
> <6>serial8250.0: ttyS0 at MMIO 0x4806a000 (irq = 72) is a ST16654
> <6>serial8250.0: ttyS1 at MMIO 0x4806e000 (irq = 74) is a ST16654
> <ó"F:­5 #!*$Õ<ó¬#0­5 #!*$Õ<ó­r%
> µ²² æ2Õ<ó$¶ #®*#5##¹'%Õ<ó#62°$¶ ¹)K!¥º¹'Õ<ó¬¹5 K#z 0«²)Ñ%f¡T:6æ0ªÿ<ó¯
> ó
> %°ö2­Õ³#²©T ô­#.#É ,­#2¬#£µ0V³/­5 V§Í#%Rí<ó00¢F ô- ¹##£$¶ +3Sí<ó
> 0
> ·#
>   # V60±:)¥#
> ,#µ¥Õ46 V#Õ#
> ¬é)¶.¹ªÿ<óµ&£É%²:©F%V$®º ¹##£$¶
>  Í#É¡*ªÿ<óµ&                            ©#­0Á:£µ­.#µ¥Õ4#Ô#Kªÿ#ó#Í!Ä)
>  %
> ³3{%°ö2­±9±Sí<ó"Õ {/+#í#ó#Í!Ä)4?+#í#ó#Í!Ä)4ó ö£U4¬¹ ¹)K!)º"ªÿ<ó"Õ%{/C:³ô
> Ù¯    s
> ®Ñ ®®Ò 335<#U2,°®!Ä)  ô2V#"ö/ë"¢#4/##®#%!V C%!

Might be related to the omap specific ATAGs now gone, sorry but that
just had to go to get things in sync. And it should not come as a
surprise as it's been discussed over past few years :)

The missing data needs to be initialized in platform_data, cmdline,
ARM generic ATAGs, upcoming device tree.. I suggest using platform_data
where possible. The GPIO values etc coming from nolo can be dumped
with some earlier kernel using CONFIG_ATAGS_PROC.

BTW, I'll move the n8x0 board-*.c files around a bit to have just
board-n800.c, board-n810.c and board-n8x0-peripherals.c.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Koskinen, Aaro (Nokia - FI/Espoo) June 15, 2009, 8:37 a.m. UTC | #4

Hello,

ext Hemanth V wrote:
> From: "Aaro Koskinen" <[e-mail]>
>> From: Eero Nurkkala <[e-mail]>
>>
>> When data is read through DMA, the last element must be read separately
>> through the RX register. It cannot be transferred by the DMA. For further
>> details see e.g. OMAP3430 TRM.
> 
> Could you provide the section number and the version of TRM you are
> referring to.

Table 19-16 and section 19.6.2.5.1 in the public TRM.

>> Without the fix the driver causes extra clocks to be clocked to the
>> bus after DMA RX operations. This can cause interesting behaviour with
>> some devices.
> 
> Could you provide more details on the errors seen i.e is the transfer 
> aborted

There is an extra word, which a device can interpret as a command. Also 
if the word size != command size the device can hang. Unless the device 
hangs, you may not even notice anything.

A.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kalle Valo June 18, 2009, 12:26 p.m. UTC | #5

Tony Lindgren <[e-mail]> writes:

>> I will test this patch as soon as I get N800 boot with latest
>> linux-omap, now I only get this:
>> 
>> <6>JFFS2 version 2.2. (NAND) (SUMMARY)  © 2001-2006 Red Hat, Inc.
>> <6>JFFS2 version 2.2. (NAND) (SUMMARY)  © 2001-2006 Red H
>> <6>msgmni has been set to 247
>> <6>io scheduler noop registered (default)
>> <6>omapfb: ls041y3 rev 87 LCD detected, 0 data lines      d
>> <6>serial8250.0: ttyS0 at MMIO 0x4806a000 (irq = 72) is a ST16654
>> <6>serial8250.0: ttyS1 at MMIO 0x4806e000 (irq = 74) is a ST16654
>> <ó"F:­5 #!*$Õ<ó¬#0­5 #!*$Õ<ó­r%
>> µ²² æ2Õ<ó$¶ #®*#5##¹'%Õ<ó#62°$¶ ¹)K!¥º¹'Õ<ó¬¹5 K#z 0«²)Ñ%f¡T:6æ0ªÿ<ó¯
>> ó
>> %°ö2­Õ³#²©T ô­#.#É ,­#2¬#£µ0V³/­5 V§Í#%Rí<ó00¢F ô- ¹##£$¶ +3Sí<ó
>> 0
>> ·#
>>   # V60±:)¥#
>> ,#µ¥Õ46 V#Õ#
>> ¬é)¶.¹ªÿ<óµ&£É%²:©F%V$®º ¹##£$¶
>>  Í#É¡*ªÿ<óµ&                            ©#­0Á:£µ­.#µ¥Õ4#Ô#Kªÿ#ó#Í!Ä)
>>  %
>> ³3{%°ö2­±9±Sí<ó"Õ {/+#í#ó#Í!Ä)4?+#í#ó#Í!Ä)4ó ö£U4¬¹ ¹)K!)º"ªÿ<ó"Õ%{/C:³ô
>> Ù¯    s
>> ®Ñ ®®Ò 335<#U2,°®!Ä)  ô2V#"ö/ë"¢#4/##®#%!V C%!
>
> Might be related to the omap specific ATAGs now gone, sorry but that
> just had to go to get things in sync. And it should not come as a
> surprise as it's been discussed over past few years :)

Yeah, I understand. Better do it properly, it will get easier soon.

> The missing data needs to be initialized in platform_data, cmdline,
> ARM generic ATAGs, upcoming device tree.. I suggest using platform_data
> where possible. The GPIO values etc coming from nolo can be dumped
> with some earlier kernel using CONFIG_ATAGS_PROC.

Thanks for the help, all hints are greatly appreciated. And as soon as I
manage to finish a personal project of mine I'll start looking at this
in detail.

> BTW, I'll move the n8x0 board-*.c files around a bit to have just
> board-n800.c, board-n810.c and board-n8x0-peripherals.c.

Sounds good.

Patch

diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
index d6d0c5d..0f640b0 100644
--- a/drivers/spi/omap2_mcspi.c
+++ b/drivers/spi/omap2_mcspi.c
@@ -269,7 +269,7 @@  omap2_mcspi_txrx_dma(struct spi_device *spi, struct spi_transfer *xfer)
 
 	if (rx != NULL) {
 		omap_set_dma_transfer_params(mcspi_dma->dma_rx_channel,
-				data_type, element_count, 1,
+				data_type, element_count - 1, 1,
 				OMAP_DMA_SYNC_ELEMENT,
 				mcspi_dma->dma_rx_sync_dev, 1);
 
@@ -300,6 +300,25 @@  omap2_mcspi_txrx_dma(struct spi_device *spi, struct spi_transfer *xfer)
 	if (rx != NULL) {
 		wait_for_completion(&mcspi_dma->dma_rx_completion);
 		dma_unmap_single(NULL, xfer->rx_dma, count, DMA_FROM_DEVICE);
+		omap2_mcspi_set_enable(spi, 0);
+		if (likely(mcspi_read_cs_reg(spi, OMAP2_MCSPI_CHSTAT0) &
+			OMAP2_MCSPI_CHSTAT_RXS)) {
+			u32 w;
+
+			w = mcspi_read_cs_reg(spi, OMAP2_MCSPI_RX0);
+			if (word_len <= 8)
+				((u8 *)xfer->rx_buf)[element_count - 1] = w;
+			else if (word_len <= 16)
+				((u16 *)xfer->rx_buf)[element_count - 1] = w;
+			else /* word_len <= 32 */
+				((u32 *)xfer->rx_buf)[element_count - 1] = w;
+		} else {
+			dev_err(&spi->dev, "DMA RX last word empty");
+			count -= (word_len <= 8)  ? 1 :
+				 (word_len <= 16) ? 2 :
+			       /* word_len <= 32 */ 4;
+		}
+		omap2_mcspi_set_enable(spi, 1);
 	}
 	return count;
 }