Skip to contentSkip to conversation

SDHCI ADMA clenaups

PATCH In review Ben Dooks 2 participants 5 messages Jan 28, 2010 – Feb 2, 2010

Subsystems (3)
Arm Samsung Exynos MMC, SD and SDIO Secure digital host controller interface (SDHCI) driver
lore
Share & tools
lore

Ben Dooks

author
#0 lore raw

ADMA code cleanps for SDHCI driver. Makes the code shorter and more efficient.

author
#1 lore raw

PATCH 1/1 sdhci: add adma descriptor set call

The code to write the ADMA descriptor into memory is repeated several times throughout sdhci_adma_table_pre, and thus should be moved into a common function. This will also be useful if the patch to make the write more efficient is accepted.

Signed-off-by Ben Dooks <[email protected]>
---
 drivers/mmc/host/sdhci.c |   51 +++++++++++++++++++---------------------------
 1 files changed, 21 insertions(+), 30 deletions(-)
drivers/mmc/host/sdhci.c +21 -30
index 8be2613..391eb08 100644
@@ -376,6 +376,20 @@ static void sdhci_kunmap_atomic(void *buffer, unsigned long *flags)
 	local_irq_restore(*flags);
 }
 
+static void sdhci_set_adma_desc(u8 *desc, u32 addr, int len, unsigned cmd)
+{
+	desc[7] = (addr >> 24) & 0xff;
+	desc[6] = (addr >> 16) & 0xff;
+	desc[5] = (addr >> 8) & 0xff;
+	desc[4] = (addr >> 0) & 0xff;
+
+	desc[3] = (len >> 8) & 0xff;
+	desc[2] = (len >> 0) & 0xff;
+
+	desc[0] = cmd & 0xff;
+	desc[1] = cmd >> 8;
+}
+
 static int sdhci_adma_table_pre(struct sdhci_host *host,
 	struct mmc_data *data)
 {

@@ -443,19 +457,11 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,
 				sdhci_kunmap_atomic(buffer, &flags);
 			}
 
-			desc[7] = (align_addr >> 24) & 0xff;
-			desc[6] = (align_addr >> 16) & 0xff;
-			desc[5] = (align_addr >> 8) & 0xff;
-			desc[4] = (align_addr >> 0) & 0xff;
+			/* tran, valid */
+			sdhci_set_adma_desc(desc, align_addr, offset, 0x21);
 
 			BUG_ON(offset > 65536);
 
-			desc[3] = (offset >> 8) & 0xff;
-			desc[2] = (offset >> 0) & 0xff;
-
-			desc[1] = 0x00;
-			desc[0] = 0x21; /* tran, valid */
-
 			align += 4;
 			align_addr += 4;
 

@@ -465,19 +471,10 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,
 			len -= offset;
 		}
 
-		desc[7] = (addr >> 24) & 0xff;
-		desc[6] = (addr >> 16) & 0xff;
-		desc[5] = (addr >> 8) & 0xff;
-		desc[4] = (addr >> 0) & 0xff;
-
 		BUG_ON(len > 65536);
 
-		desc[3] = (len >> 8) & 0xff;
-		desc[2] = (len >> 0) & 0xff;
-
-		desc[1] = 0x00;
-		desc[0] = 0x21; /* tran, valid */
-
+		/* tran, valid */
+		sdhci_set_adma_desc(desc, addr, len, 0x21);
 		desc += 8;
 
 		/*

@@ -499,15 +496,9 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,
 		/*
 		* Add a terminating entry.
 		*/
-		desc[7] = 0;
-		desc[6] = 0;
-		desc[5] = 0;
-		desc[4] = 0;
-
-		desc[3] = 0;
-		desc[2] = 0;
-		desc[1] = 0x00;
-		desc[0] = 0x03; /* nop, end, valid */
+
+		/* nop, end, valid */
+		sdhci_set_adma_desc(desc, 0, 0, 0x3);
 	}
 
 	/*
Signature · 1 line
1.6.0.4
#2 lore raw

Re: [PATCH] sdhci: add adma descriptor set call

Quoted 14 lines · On Thu, 28 Jan 2010 05:29:56 +0000 Ben Dooks <[email protected]> wrote:

The code to write the ADMA descriptor into memory is repeated several times throughout sdhci_adma_table_pre, and thus should be moved into a common function. This will also be useful if the patch to make the write more efficient is accepted.

...

drivers/mmc/host/sdhci.c +0 -3
@@ -499,15 +496,9 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,
 		/*
 		* Add a terminating entry.
 		*/
-		desc[7] = 0;
-		desc[6] = 0;
-		desc[5] = 0;

drivers/mmc/host/sdhci.c static int sdhci_adma_table_pre(struct sdhci_host *host, · line 502

Quoted 10 lines
drivers/mmc/host/sdhci.c +3 -6
-		desc[4] = 0;
-
-		desc[3] = 0;
-		desc[2] = 0;
-		desc[1] = 0x00;
-		desc[0] = 0x03; /* nop, end, valid */
+
+		/* nop, end, valid */
+		sdhci_set_adma_desc(desc, 0, 0, 0x3);
 	}

What kernel are you patching? Current mainline is different from the above.

author
#3 lore raw

Re: [PATCH] sdhci: add adma descriptor set call

Quoted 21 lines · Andrew Morton
Quoted 18 lines · On Thu, 28 Jan 2010 05:29:56 +0000 Ben Dooks <[email protected]> wrote:

The code to write the ADMA descriptor into memory is repeated several times throughout sdhci_adma_table_pre, and thus should be moved into a common function. This will also be useful if the patch to make the write more efficient is accepted.

...

drivers/mmc/host/sdhci.c +0 -7
@@ -499,15 +496,9 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,
 		/*
 		* Add a terminating entry.
 		*/
-		desc[7] = 0;
-		desc[6] = 0;
-		desc[5] = 0;
-		desc[4] = 0;
-
-		desc[3] = 0;
-		desc[2] = 0;
Quoted 8 lines

drivers/mmc/host/sdhci.c static int sdhci_adma_table_pre(struct sdhci_host *host, · line 502

Quoted 6 lines
drivers/mmc/host/sdhci.c +3 -2
-		desc[1] = 0x00;
-		desc[0] = 0x03; /* nop, end, valid */
+
+		/* nop, end, valid */
+		sdhci_set_adma_desc(desc, 0, 0, 0x3);
 	}

What kernel are you patching? Current mainline is different from the above.

This follows Thomas Abraham's ADMA end fixup quirk.

Signature · 4 lines
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.
author
#4 lore raw

PATCH 1/1 sdhci: improve sdhci sdhci_set_adma_desc() code

In reply to Ben Dooks

The sdhci_set_adma_desc is using byte-writes to write data in a specified order into memory. Change to using __le16 for the two byte and __le32 for the four byte cases and use the cpu_to_{le16,le32} to do the conversion before writing.

This will reduce the size of the code and the number of writes as we no longer need to chop the data up before writing.

As an example on ARM S3C64XX SoC, in little-endian configuration:

000000d4 <sdhci_set_adma_desc>:

-      d8:	e1a0c423 	lsr	ip, r3, #8
-      dc:	e1a0ec21 	lsr	lr, r1, #24
-      e0:	e1a04821 	lsr	r4, r1, #16
-      e4:	e1a05421 	lsr	r5, r1, #8
-      e8:	e1a06442 	asr	r6, r2, #8
-      ec:	e5c0c001 	strb	ip, [r0, #1]
-      f0:	e5c0e007 	strb	lr, [r0, #7]
-      f4:	e5c04006 	strb	r4, [r0, #6]
-      f8:	e5c05005 	strb	r5, [r0, #5]
-      fc:	e5c01004 	strb	r1, [r0, #4]
-     100:	e5c06003 	strb	r6, [r0, #3]
-     104:	e5c02002 	strb	r2, [r0, #2]
-     108:	e5c03000 	strb	r3, [r0]
+      d4:	e5801004 	str	r1, [r0, #4]
+      d8:	e1c030b0 	strh	r3, [r0]
+      dc:	e1c020b2 	strh	r2, [r0, #2]
Signed-off-by Ben Dooks <[email protected]>
---
 drivers/mmc/host/sdhci.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)
drivers/mmc/host/sdhci.c +8 -8
index 391eb08..d09a1d5 100644
@@ -378,16 +378,16 @@ static void sdhci_kunmap_atomic(void *buffer, unsigned long *flags)
 
 static void sdhci_set_adma_desc(u8 *desc, u32 addr, int len, unsigned cmd)
 {
-	desc[7] = (addr >> 24) & 0xff;
-	desc[6] = (addr >> 16) & 0xff;
-	desc[5] = (addr >> 8) & 0xff;
-	desc[4] = (addr >> 0) & 0xff;
+	__le32 *dataddr = (__le32 __force *)(desc + 4);
+	__le16 *cmdlen = (__le16 __force *)desc;
 
-	desc[3] = (len >> 8) & 0xff;
-	desc[2] = (len >> 0) & 0xff;
+	/* SDHCI specification says ADMA descriptors should be 4 byte
+	 * aligned, so using 16 or 32bit operations should be safe. */
 
-	desc[0] = cmd & 0xff;
-	desc[1] = cmd >> 8;
+	cmdlen[0] = cpu_to_le16(cmd);
+	cmdlen[1] = cpu_to_le16(len);
+
+	dataddr[0] = cpu_to_le32(addr);
 }
 
 static int sdhci_adma_table_pre(struct sdhci_host *host,
Signature · 1 line
1.6.0.4