Skip to contentSkip to conversation

MX28: DMA: Align the struct mxs_dma_desc

PATCH New Marek Vasut 5 participants 13 messages Aug 22, 2012 – Aug 27, 2012

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

PATCH 1/5 MX28: DMA: Align the struct mxs_dma_desc

Align this structure to DMA alignment size.

Cc Fabio Estevam <[email protected]>
Cc Otavio Salvador <[email protected]>
Cc Stefano Babic <[email protected]>
---
 arch/arm/include/asm/arch-mxs/dma.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/arm/include/asm/arch-mxs/dma.h +2 -1
index 4a1820b..a0a0ea5 100644
@@ -27,6 +27,7 @@
 #define __DMA_H__
 
 #include <linux/list.h>
+#include <linux/compiler.h>
 
 #ifndef	CONFIG_ARCH_DMA_PIO_WORDS
 #define	DMA_PIO_WORDS		15
@@ -109,7 +110,7 @@ struct mxs_dma_desc {
 	dma_addr_t		address;
 	void			*buffer;
 	struct list_head	node;
-};
+} __aligned(MXS_DMA_ALIGNMENT);
 
 /**
  * MXS DMA channel
Signature · 1 line
1.7.10.4
#1 lore raw

PATCH 2/5 MX28: DMA: Prolong the DMA timeout

Load from SPI flash can create a long DMA chain, which can take long time to transfer. Change the DMA timeout to roughly 10s to prevent such long chains misreporting errors.

Cc Fabio Estevam <[email protected]>
Cc Otavio Salvador <[email protected]>
Cc Stefano Babic <[email protected]>
---
 drivers/dma/apbh_dma.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/dma/apbh_dma.c +1 -1
index ca5a32f..37a941c 100644
@@ -526,7 +526,7 @@ static int mxs_dma_wait_complete(uint32_t timeout, unsigned int chan)
  */
 int mxs_dma_go(int chan)
 {
-	uint32_t timeout = 10000;
+	uint32_t timeout = 10000000;
 	int ret;
 
 	LIST_HEAD(tmp_desc_list);
Signature · 1 line
1.7.10.4
reviewer
#2 lore raw

PATCH 2/5 MX28: DMA: Prolong the DMA timeout

drivers/dma/apbh_dma.c static int mxs_dma_wait_complete(uint32_t timeout, unsigned int chan) · line 529

Quoted 4 lines · Marek Vasut

int mxs_dma_go(int chan)
{

drivers/dma/apbh_dma.c +1 -1
-       uint32_t timeout = 10000;
+       uint32_t timeout = 10000000;

Should we use a proper timeout mechanism instead?

Reagards,

Fabio Estevam

#3 lore raw

PATCH 2/5 MX28: DMA: Prolong the DMA timeout

Dear Fabio Estevam,

Quoted 8 lines · Marek Vasut

drivers/dma/apbh_dma.c static int mxs_dma_wait_complete(uint32_t timeout, unsigned int chan) · line 529

Quoted 5 lines

int mxs_dma_go(int chan)
{

drivers/dma/apbh_dma.c +1 -1
-       uint32_t timeout = 10000;
+       uint32_t timeout = 10000000;

Should we use a proper timeout mechanism instead?

What would that be? I think 10 seconds is more than plenty for now.

Quoted 3 lines

Reagards,

Fabio Estevam

Best regards,
Marek Vasut

reviewer
#4 lore raw

PATCH 2/5 MX28: DMA: Prolong the DMA timeout

Quoted 6 lines · Marek Vasut

Dear Fabio Estevam,

Quoted 4 lines · Marek Vasut
Quoted 3 lines

int mxs_dma_go(int chan)
{

Quoted 6 lines
Quoted 4 lines

Review

Quoted 2 lines
(quoted diff) +1 -1
-       uint32_t timeout = 10000;
+       uint32_t timeout = 10000000;

Should we use a proper timeout mechanism instead?

What would that be? I think 10 seconds is more than plenty for now.

Ok, but we need to change to unsigned int to be able to fit 10000000:

arch/arm/cpu/arm926ejs/mxs/mxs.c +4 -2
@@ -81,7 +81,8 @@ void enable_caches(void)
 #endif
 }
 
-int mxs_wait_mask_set(struct mxs_register_32 *reg, uint32_t mask, int timeout)
+int mxs_wait_mask_set(struct mxs_register_32 *reg, uint32_t mask, unsigned
+                                                               int timeout)
 {
        while (--timeout) {
                if ((readl(&reg->reg) & mask) == mask)
@@ -92,7 +93,8 @@ int mxs_wait_mask_set(struct mxs_register_32 *reg, uint32_t ma
        return !timeout;
 }
 
-int mxs_wait_mask_clr(struct mxs_register_32 *reg, uint32_t mask, int timeout)
+int mxs_wait_mask_clr(struct mxs_register_32 *reg, uint32_t mask, unsigned
+                                                               int timeout)
 {
        while (--timeout) {
                if ((readl(&reg->reg) & mask) == 0)
arch/arm/include/asm/arch-mxs/sys_proto.h +2 -2
index 4610363..983b888 100644
@@ -26,10 +26,10 @@
 int mxs_reset_block(struct mxs_register_32 *reg);
 int mxs_wait_mask_set(struct mxs_register_32 *reg,
                       uint32_t mask,
-                      int timeout);
+                      unsigned int timeout);
 int mxs_wait_mask_clr(struct mxs_register_32 *reg,
                       uint32_t mask,
-                      int timeout);
+                      unsigned int timeout);
 
 int mxsmmc_initialize(bd_t *bis, int id, int (*wp)(int));
 

Regards,

Fabio Estevam

#5 lore raw

PATCH 2/5 MX28: DMA: Prolong the DMA timeout

Dear Fabio Estevam,

Quoted 7 lines · Marek Vasut
Quoted 6 lines

Dear Fabio Estevam,

Quoted 4 lines · Marek Vasut
Quoted 3 lines

int mxs_dma_go(int chan)
{

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

Review

Quoted 2 lines
(quoted diff) +1 -1
-       uint32_t timeout = 10000;
+       uint32_t timeout = 10000000;

Should we use a proper timeout mechanism instead?

What would that be? I think 10 seconds is more than plenty for now.

Ok, but we need to change to unsigned int to be able to fit 10000000:

Do we? 10M still fits in ... but it's indeed a good idea, can you submit a patch for that please?

Quoted 41 lines
arch/arm/cpu/arm926ejs/mxs/mxs.c +0 -1
@@ -81,7 +81,8 @@ void enable_caches(void)
 #endif
 }
 
-int mxs_wait_mask_set(struct mxs_register_32 *reg, uint32_t mask, int

timeout) +int mxs_wait_mask_set(struct mxs_register_32 *reg, uint32_t mask, unsigned +

    int timeout) {
        while (--timeout) {
                if ((readl(&reg->reg) & mask) == mask)
arch/arm/cpu/arm926ejs/mxs/mxs.c +0 -1
@@ -92,7 +93,8 @@ int mxs_wait_mask_set(struct mxs_register_32 *reg,
 uint32_t ma return !timeout;
 }
 
-int mxs_wait_mask_clr(struct mxs_register_32 *reg, uint32_t mask, int

timeout) +int mxs_wait_mask_clr(struct mxs_register_32 *reg, uint32_t mask, unsigned +

    int timeout) {
        while (--timeout) {
                if ((readl(&reg->reg) & mask) == 0)
(quoted diff) +0 -0

b/arch/arm/include/asm/ar index 4610363..983b888 100644

arch/arm/include/asm/arch-mxs/sys_proto.h +2 -2
@@ -26,10 +26,10 @@
 int mxs_reset_block(struct mxs_register_32 *reg);
 int mxs_wait_mask_set(struct mxs_register_32 *reg,
                       uint32_t mask,
-                      int timeout);
+                      unsigned int timeout);
 int mxs_wait_mask_clr(struct mxs_register_32 *reg,
                       uint32_t mask,
-                      int timeout);
+                      unsigned int timeout);
 
 int mxsmmc_initialize(bd_t *bis, int id, int (*wp)(int));
 

Regards,

Quoted 1 line

Fabio Estevam

Best regards,
Marek Vasut

#6 lore raw

PATCH 3/5 MX28: SPI: Supercharge the SPI driver

In reply to Marek Vasut

This change implements DMA chaining into SPI driver. This allows the transfers to go much faster, while also fixing SF issues.

Cc Fabio Estevam <[email protected]>
Cc Otavio Salvador <[email protected]>
Cc Stefano Babic <[email protected]>
---
 drivers/spi/mxs_spi.c |   96 +++++++++++++++++++++++++++++++++----------------
 1 file changed, 65 insertions(+), 31 deletions(-)
drivers/spi/mxs_spi.c +65 -31
index a037c13..fef4302 100644
@@ -56,7 +56,6 @@ struct mxs_spi_slave {
 	uint32_t		max_khz;
 	uint32_t		mode;
 	struct mxs_ssp_regs	*regs;
-	struct mxs_dma_desc	*desc;
 };
 
 static inline struct mxs_spi_slave *to_mxs_slave(struct spi_slave *slave)
@@ -84,7 +83,6 @@ struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
 	uint32_t addr;
 	struct mxs_ssp_regs *ssp_regs;
 	int reg;
-	struct mxs_dma_desc *desc;
 
 	if (!spi_cs_is_valid(bus, cs)) {
 		printf("mxs_spi: invalid bus %d / chip select %d\n", bus, cs);
@@ -95,10 +93,6 @@ struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
 	if (!mxs_slave)
 		return NULL;
 
-	desc = mxs_dma_desc_alloc();
-	if (!desc)
-		goto err_desc;
-
 	if (mxs_dma_init_channel(bus))
 		goto err_init;
 
@@ -109,7 +103,6 @@ struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
 	mxs_slave->max_khz = max_hz / 1000;
 	mxs_slave->mode = mode;
 	mxs_slave->regs = (struct mxs_ssp_regs *)addr;
-	mxs_slave->desc = desc;
 	ssp_regs = mxs_slave->regs;
 
 	reg = readl(&ssp_regs->hw_ssp_ctrl0);
@@ -120,8 +113,6 @@ struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
 	return &mxs_slave->slave;
 
 err_init:
-	mxs_dma_desc_free(desc);
-err_desc:
 	free(mxs_slave);
 	return NULL;
 }
@@ -129,7 +120,6 @@ err_desc:
 void spi_free_slave(struct spi_slave *slave)
 {
 	struct mxs_spi_slave *mxs_slave = to_mxs_slave(slave);
-	mxs_dma_desc_free(mxs_slave->desc);
 	free(mxs_slave);
 }
 
@@ -228,19 +218,24 @@ static int mxs_spi_xfer_pio(struct mxs_spi_slave *slave,
 static int mxs_spi_xfer_dma(struct mxs_spi_slave *slave,
 			char *data, int length, int write, unsigned long flags)
 {
-	struct mxs_dma_desc *desc = slave->desc;
+	const int xfer_max_sz = 0xff00;
+	const int desc_count = DIV_ROUND_UP(length, xfer_max_sz) + 1;
 	struct mxs_ssp_regs *ssp_regs = slave->regs;
-	uint32_t ctrl0 = SSP_CTRL0_DATA_XFER;
+	struct mxs_dma_desc *dp;
+	uint32_t ctrl0;
 	uint32_t cache_data_count;
 	int dmach;
+	int tl;
+
+	ALLOC_CACHE_ALIGN_BUFFER(struct mxs_dma_desc, desc, desc_count);
+
+	memset(desc, 0, sizeof(struct mxs_dma_desc) * desc_count);
 
-	memset(desc, 0, sizeof(struct mxs_dma_desc));
-	desc->address = (dma_addr_t)desc;
+	ctrl0 = readl(&ssp_regs->hw_ssp_ctrl0);
+	ctrl0 |= SSP_CTRL0_DATA_XFER;
 
 	if (flags & SPI_XFER_BEGIN)
 		ctrl0 |= SSP_CTRL0_LOCK_CS;
-	if (flags & SPI_XFER_END)
-		ctrl0 |= SSP_CTRL0_IGNORE_CRC;
 	if (!write)
 		ctrl0 |= SSP_CTRL0_READ;
 
@@ -251,27 +246,66 @@ static int mxs_spi_xfer_dma(struct mxs_spi_slave *slave,
 	else
 		cache_data_count = length;
 
-	if (!write) {
-		slave->desc->cmd.data = MXS_DMA_DESC_COMMAND_DMA_WRITE;
-		slave->desc->cmd.address = (dma_addr_t)data;
-	} else {
-		slave->desc->cmd.data = MXS_DMA_DESC_COMMAND_DMA_READ;
-		slave->desc->cmd.address = (dma_addr_t)data;
-
+	if (write)
 		/* Flush data to DRAM so DMA can pick them up */
 		flush_dcache_range((uint32_t)data,
 			(uint32_t)(data + cache_data_count));
-	}
 
-	slave->desc->cmd.data |= MXS_DMA_DESC_IRQ | MXS_DMA_DESC_DEC_SEM |
-				(length << MXS_DMA_DESC_BYTES_OFFSET) |
-				(1 << MXS_DMA_DESC_PIO_WORDS_OFFSET) |
-				MXS_DMA_DESC_WAIT4END;
+	dmach = MXS_DMA_CHANNEL_AHB_APBH_SSP0 + slave->slave.bus;
 
-	slave->desc->cmd.pio_words[0] = ctrl0;
+	dp = desc;
+	while (length) {
+		dp->address = (dma_addr_t)dp;
+		dp->cmd.address = (dma_addr_t)data;
+
+		/*
+		 * This is correct, even though it does indeed look insane.
+		 * I hereby have to, wholeheartedly, thank Freescale Inc.,
+		 * for always inventing insane hardware and keeping me busy
+		 * and employed ;-)
+		 */
+		if (write)
+			dp->cmd.data = MXS_DMA_DESC_COMMAND_DMA_READ;
+		else
+			dp->cmd.data = MXS_DMA_DESC_COMMAND_DMA_WRITE;
+
+		/*
+		 * The DMA controller can transfer large chunks (64kB) at
+		 * time by setting the transfer length to 0. Setting tl to
+		 * 0x10000 will overflow below and make .data contain 0.
+		 * Otherwise, 0xff00 is the transfer maximum.
+		 */
+		if (length >= 0x10000)
+			tl = 0x10000;
+		else
+			tl = min(length, xfer_max_sz);
+
+		dp->cmd.data |=
+			(tl << MXS_DMA_DESC_BYTES_OFFSET) |
+			(1 << MXS_DMA_DESC_PIO_WORDS_OFFSET) |
+			MXS_DMA_DESC_HALT_ON_TERMINATE |
+			MXS_DMA_DESC_TERMINATE_FLUSH;
+		dp->cmd.pio_words[0] = ctrl0;
+
+		data += tl;
+		length -= tl;
+
+		mxs_dma_desc_append(dmach, dp);
+
+		dp++;
+	}
+
+	dp->address = (dma_addr_t)dp;
+	dp->cmd.address = (dma_addr_t)0;
+	dp->cmd.data = MXS_DMA_DESC_COMMAND_NO_DMAXFER |
+			(1 << MXS_DMA_DESC_PIO_WORDS_OFFSET) |
+			MXS_DMA_DESC_IRQ | MXS_DMA_DESC_DEC_SEM;
+	if (flags & SPI_XFER_END) {
+		ctrl0 &= ~SSP_CTRL0_LOCK_CS;
+		dp->cmd.pio_words[0] = ctrl0 | SSP_CTRL0_IGNORE_CRC;
+	}
+	mxs_dma_desc_append(dmach, dp);
 
-	dmach = MXS_DMA_CHANNEL_AHB_APBH_SSP0 + slave->slave.bus;
-	mxs_dma_desc_append(dmach, slave->desc);
 	if (mxs_dma_go(dmach))
 		return -EINVAL;
 
Signature · 1 line
1.7.10.4
#7 lore raw

PATCH 3/5 MX28: SPI: Supercharge the SPI driver

Quoted 2 lines · On Tuesday 21 August 2012 22:17:27 Marek Vasut wrote:

This change implements DMA chaining into SPI driver. This allows the transfers to go much faster, while also fixing SF issues.

might be nice to have the word "DMA" in the patch summary

MX28: SPI: Supercharge the SPI driver w/DMA
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120822/507d494b/attachment.pgp>

#8 lore raw

PATCH 3/5 MX28: SPI: Supercharge the SPI driver

Dear Mike Frysinger,

Quoted 8 lines · On Tuesday 21 August 2012 22:17:27 Marek Vasut wrote:
Quoted 2 lines

This change implements DMA chaining into SPI driver. This allows the transfers to go much faster, while also fixing SF issues.

might be nice to have the word "DMA" in the patch summary

MX28: SPI: Supercharge the SPI driver w/DMA
-mike

Possibly ... it's obvious from the description though

Best regards,
Marek Vasut

#9 lore raw

PATCH 3/5 MX28: SPI: Supercharge the SPI driver

Quoted 10 lines · On Wednesday 22 August 2012 13:43:19 Marek Vasut wrote:

Dear Mike Frysinger,

Quoted 7 lines · On Tuesday 21 August 2012 22:17:27 Marek Vasut wrote:
Quoted 2 lines

This change implements DMA chaining into SPI driver. This allows the transfers to go much faster, while also fixing SF issues.

might be nice to have the word "DMA" in the patch summary

MX28: SPI: Supercharge the SPI driver w/DMA

Possibly ... it's obvious from the description though

yes, the description makes it obvious, if you get that far. but the summary is useless when scanning with git log --format=oneline, or even just git log and letting the eye track the subject line.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120822/33a77a8c/attachment.pgp>

#10 lore raw

PATCH 4/5 MX28: m28evk: Align SSP clock speed

In reply to Marek Vasut

Align the SSP clock speed with oscilator to achieve higher transfer stability.

Cc Fabio Estevam <[email protected]>
Cc Otavio Salvador <[email protected]>
Cc Stefano Babic <[email protected]>
---
 board/denx/m28evk/m28evk.c |    4 ++--
 include/configs/m28evk.h   |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)
board/denx/m28evk/m28evk.c +2 -2
index 74da3ea..255c139 100644
@@ -49,8 +49,8 @@ int board_early_init_f(void)
 
 	/* SSP0 clock at 96MHz */
 	mx28_set_sspclk(MXC_SSPCLK0, 96000, 0);
-	/* SSP2 clock at 96MHz */
-	mx28_set_sspclk(MXC_SSPCLK2, 96000, 0);
+	/* SSP2 clock at 160MHz */
+	mx28_set_sspclk(MXC_SSPCLK2, 160000, 0);
 
 #ifdef	CONFIG_CMD_USB
 	mxs_iomux_setup_pad(MX28_PAD_SSP2_SS1__USB1_OVERCURRENT);
include/configs/m28evk.h +2 -2
index b3ac316..2b16d69 100644
@@ -255,11 +255,11 @@
 #define	CONFIG_SPI_FLASH_STMICRO
 #define	CONFIG_SF_DEFAULT_CS		2
 #define	CONFIG_SF_DEFAULT_MODE		SPI_MODE_0
-#define	CONFIG_SF_DEFAULT_SPEED		24000000
+#define	CONFIG_SF_DEFAULT_SPEED		40000000
 
 #define	CONFIG_ENV_SPI_CS		0
 #define	CONFIG_ENV_SPI_BUS		2
-#define	CONFIG_ENV_SPI_MAX_HZ		24000000
+#define	CONFIG_ENV_SPI_MAX_HZ		40000000
 #define	CONFIG_ENV_SPI_MODE		SPI_MODE_0
 #endif
 #endif
Signature · 1 line
1.7.10.4
#11 lore raw

PATCH 5/5 MX28: m28evk: Enable SPI DMA

In reply to Marek Vasut
Cc Fabio Estevam <[email protected]>
Cc Otavio Salvador <[email protected]>
Cc Stefano Babic <[email protected]>
---
 include/configs/m28evk.h |    1 +
 1 file changed, 1 insertion(+)
include/configs/m28evk.h +1 -0
index 2b16d69..4e9758f 100644
@@ -245,6 +245,7 @@
 #ifdef	CONFIG_CMD_SPI
 #define	CONFIG_HARD_SPI
 #define	CONFIG_MXS_SPI
+#define	CONFIG_MXS_SPI_DMA_ENABLE
 #define	CONFIG_SPI_HALF_DUPLEX
 #define	CONFIG_DEFAULT_SPI_BUS		2
 #define	CONFIG_DEFAULT_SPI_MODE		SPI_MODE_0
Signature · 1 line
1.7.10.4
#12 lore raw

PATCH 1/5 MX28: DMA: Align the struct mxs_dma_desc

In reply to Marek Vasut
Quoted 24 lines · Marek Vasut

Align this structure to DMA alignment size.

Cc Fabio Estevam <[email protected]>
Cc Otavio Salvador <[email protected]>
Cc Stefano Babic <[email protected]>
---
 arch/arm/include/asm/arch-mxs/dma.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/arm/include/asm/arch-mxs/dma.h +1 -0
index 4a1820b..a0a0ea5 100644
@@ -27,6 +27,7 @@
 #define __DMA_H__
 
 #include <linux/list.h>
+#include <linux/compiler.h>
 
 #ifndef	CONFIG_ARCH_DMA_PIO_WORDS
 #define	DMA_PIO_WORDS		15

arch/arm/include/asm/arch-mxs/dma.h struct mxs_dma_desc { · line 116

Quoted 8 lines
arch/arm/include/asm/arch-mxs/dma.h +1 -1
@@ -109,7 +110,7 @@ struct mxs_dma_desc {
 	dma_addr_t		address;
 	void			*buffer;
 	struct list_head	node;
-};
+} __aligned(MXS_DMA_ALIGNMENT);
 
 /**
  * MXS DMA channel

Applied (whole series) to u-boot-imx, thanks.

Best regards,
Stefano Babic

Signature · 5 lines
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================