RFT: mmc: sdhci: Implement an SDHCI-specific bounce buffer

Message ID 20180104201045.31139-1-linus.walleij@linaro.org
State New
Headers show
Series
  • RFT: mmc: sdhci: Implement an SDHCI-specific bounce buffer
Related show

Commit Message

Linus Walleij Jan. 4, 2018, 8:10 p.m.
The bounce buffer is gone from the MMC core, and now we found out
that there are some (crippled) i.MX boards out there that have broken
ADMA (cannot do scatter-gather), and broken PIO so they must use
SDMA.

SDMA sets down the number of segments to one, so that each segment
gets turned into a singular request that ping-pongs to the block
layer before the next request/segment is issued.

These devices can see major benefits from a bounce buffer, as
a fragmented read or write buffer may come in even though the sectors
we will be reading or writing to the MMC/SD-card are consecutive.

This patch accumulates those fragmented scatterlists in a physically
contigous bounce buffer so that we can issue bigger DMA data chunks
to/from the card.

It is possible to achieve even better speed-ups by adding a second
bounce buffer so that the ->pre_req() hook in the driver can do
the buffer copying and DMA mapping/flushing while the request is
in flight. We save this optimization for later.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
I would be surprised if this works, which is why i sprinkled a
few extra prints all over the place so we can see what goes
wrong. If it would happen to work, try to comment out the noisy
prints ("bouncing read/write data") so they don't disturb test
results.
---
 drivers/mmc/host/sdhci.c | 118 +++++++++++++++++++++++++++++++++++++++++++----
 drivers/mmc/host/sdhci.h |   3 ++
 2 files changed, 113 insertions(+), 8 deletions(-)

-- 
2.14.3

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

Comments

Benjamin Beckmeyer Jan. 5, 2018, 7:51 a.m. | #1
> I would be surprised if this works, which is why i sprinkled a

> few extra prints all over the place so we can see what goes

> wrong. If it would happen to work, try to comment out the noisy

> prints ("bouncing read/write data") so they don't disturb test

> results.


I can comfort you, it doesn't work :). I tried both, with your print 
outs and without. After I patched the kernel it doesn't find the root
filesystem anymore. Here is the kernel log:

sdhci: Secure Digital Host Controller Interface driver
sdhci: Copyright(c) Pierre Ossman
sdhci-pltfm: SDHCI platform and OF driver helper
SDMA with bounce buffer: bounce up to 1024 segments into one, max segment size 524288 bytes
mmc0: SDHCI controller on 53fb4000.esdhc [53fb4000.esdhc] using DMA
sdhci-esdhc-imx 53fb8000.esdhc: Got CD GPIO
sdhci-esdhc-imx 53fb8000.esdhc: Got WP GPIO
SDMA with bounce buffer: bounce up to 1024 segments into one, max segment size 524288 bytes
mmc1: SDHCI controller on 53fb8000.esdhc [53fb8000.esdhc] using DMA
bouncing read data
bouncing read data
mmc0: new high speed MMC card at address 0001
oprofile: no performance counters
mmcblk0: mmc0:0001 002G00 1.83 GiB 
mmcblk0boot0: mmc0:0001 002G00 partition 1 512 KiB
mmcblk0boot1: mmc0:0001 002G00 partition 2 512 KiB
mmcblk0rpmb: mmc0:0001 002G00 partition 3 128 KiB
bouncing read data
bouncing read data
bouncing read data
bouncing read data
bouncing read data
bouncing read data
bouncing read data
 mmcblk0: p1 p2 p3 p4 < p5 p6 p7 p8 p9 p10 >
oprofile: using timer interrupt.
NET: Registered protocol family 10
Segment Routing with IPv6
sit: IPv6, IPv4 and MPLS over IPv4 tunneling driver
NET: Registered protocol family 17
can: controller area network core (rev 20170425 abi 9)
NET: Registered protocol family 29
can: raw protocol (rev 20170425)
can: broadcast manager protocol (rev 20170425 t)
can: netlink gateway (rev 20170425) max_hops=1
input: gpio-keys as /devices/platform/gpio-keys/input/input1
imxdi_rtc 53ffc000.dryice: setting system clock to 1970-01-03 00:50:58 UTC (175858)
bouncing read data
EXT4-fs (mmcblk0p8): couldn't mount as ext3 due to feature incompatibilities
bouncing read data
EXT4-fs (mmcblk0p8): couldn't mount as ext2 due to feature incompatibilities
bouncing read data
bouncing read data
EXT4-fs (mmcblk0p8): ext4_check_descriptors: Block bitmap for group 6 not in group (block 0)!
EXT4-fs (mmcblk0p8): group descriptors corrupted!
VFS: Cannot open root device "mmcblk0p8" or unknown-block(259,0): error -117
Please append a correct "root=" boot option; here are the available partitions:
b300         1916928 mmcblk0 
 driver: mmcblk
  b301             767 mmcblk0p1 00000000-01

  b302             128 mmcblk0p2 00000000-02

  b303             128 mmcblk0p3 00000000-03

  b304               1 mmcblk0p4 

  b305           16384 mmcblk0p5 00000000-05

  b306          409599 mmcblk0p6 00000000-06

  b307           16383 mmcblk0p7 00000000-07

  103:00000     409599 mmcblk0p8 00000000-08

  103:00001     102399 mmcblk0p9 00000000-09

  103:00002     961535 mmcblk0p10 00000000-0a

b318             128 mmcblk0rpmb 
 (driver?)
b310             512 mmcblk0boot1 
 (driver?)
b308             512 mmcblk0boot0 
 (driver?)
Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(259,0)
---[ end Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(259,0)
random: crng init done

Suprisingly there is a mmcblk0rpmb partition now. I don't know what that is 
and if this was there before, but normally it has nothing to do 
with our system. On our other systems it is not present. 

After I flashed another kernel without your patch it's booting without error. 




--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Jan. 5, 2018, 9:06 a.m. | #2
On Fri, Jan 5, 2018 at 8:51 AM, Benjamin Beckmeyer
<beckmeyer.b@rittal.de> wrote:

> I can comfort you, it doesn't work :). I tried both, with your print

> outs and without.


Hey it doesn't segfault! :D

> After I patched the kernel it doesn't find the root

> filesystem anymore. Here is the kernel log:


OK I clearly need to find a system with sdhci on it, so I dug up a laptop
with SDHCI to experiment on. Now trying to figure out a bit about
x86_64 development...

> Suprisingly there is a mmcblk0rpmb partition now. I don't know what that is

> and if this was there before, but normally it has nothing to do

> with our system. On our other systems it is not present.


It is a feature of the eMMC card, and the feature list is retrieved with
special commands from the card. Usually eMMCs that have boot partitions
also have an RPMB partition.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Beckmeyer Jan. 5, 2018, 10:03 a.m. | #3
> Hey it doesn't segfault! :D


Okay, so I guess it was a successfully test :D.

> OK I clearly need to find a system with sdhci on it, so I dug up a laptop

> with SDHCI to experiment on. Now trying to figure out a bit about

> x86_64 development...


Oh, sounds like fun. I'm looking forward to read from you.

> > Suprisingly there is a mmcblk0rpmb partition now. I don't know what that is

> > and if this was there before, but normally it has nothing to do

> > with our system. On our other systems it is not present.


> It is a feature of the eMMC card, and the feature list is retrieved with

> special commands from the card. Usually eMMCs that have boot partitions

> also have an RPMB partition.


Thanks for that information, I assume it comes with a newer kernel because with
our old 3.7.2 kernel there was nothing like that.

Kind regards,
Benjamin Beckmeyer
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Jan. 5, 2018, 10:45 a.m. | #4
On Fri, Jan 5, 2018 at 11:03 AM, Benjamin Beckmeyer
<beckmeyer.b@rittal.de> wrote:
>> Hey it doesn't segfault! :D

>

> Okay, so I guess it was a successfully test :D.

>

>> OK I clearly need to find a system with sdhci on it, so I dug up a laptop

>> with SDHCI to experiment on. Now trying to figure out a bit about

>> x86_64 development...

>

> Oh, sounds like fun. I'm looking forward to read from you.

>

>> > Suprisingly there is a mmcblk0rpmb partition now. I don't know what that is

>> > and if this was there before, but normally it has nothing to do

>> > with our system. On our other systems it is not present.

>

>> It is a feature of the eMMC card, and the feature list is retrieved with

>> special commands from the card. Usually eMMCs that have boot partitions

>> also have an RPMB partition.

>

> Thanks for that information, I assume it comes with a newer kernel because with

> our old 3.7.2 kernel there was nothing like that.


Actually the patch works (!) with my laptop!

It also has only SDMA as it turns out.

I'm working on trying to figure out the problem....

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Jan. 5, 2018, 10:46 a.m. | #5
On Fri, Jan 5, 2018 at 11:45 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Jan 5, 2018 at 11:03 AM, Benjamin Beckmeyer

> <beckmeyer.b@rittal.de> wrote:

>>> Hey it doesn't segfault! :D

>>

>> Okay, so I guess it was a successfully test :D.

>>

>>> OK I clearly need to find a system with sdhci on it, so I dug up a laptop

>>> with SDHCI to experiment on. Now trying to figure out a bit about

>>> x86_64 development...

>>

>> Oh, sounds like fun. I'm looking forward to read from you.

>>

>>> > Suprisingly there is a mmcblk0rpmb partition now. I don't know what that is

>>> > and if this was there before, but normally it has nothing to do

>>> > with our system. On our other systems it is not present.

>>

>>> It is a feature of the eMMC card, and the feature list is retrieved with

>>> special commands from the card. Usually eMMCs that have boot partitions

>>> also have an RPMB partition.

>>

>> Thanks for that information, I assume it comes with a newer kernel because with

>> our old 3.7.2 kernel there was nothing like that.

>

> Actually the patch works (!) with my laptop!

>

> It also has only SDMA as it turns out.

>

> I'm working on trying to figure out the problem....


When you say you flashed "another kernel and it works" did you mean
some random kernel, or exactly the same you applied my patch on,
but without my patch?

Please confirm that you have a booting merge base so I'm not chasing
ghosts here :)

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Beckmeyer Jan. 5, 2018, 11:08 a.m. | #6
> When you say you flashed "another kernel and it works" did you mean

> some random kernel, or exactly the same you applied my patch on,

> but without my patch?


> Please confirm that you have a booting merge base so I'm not chasing

> ghosts here :)


Sorry for not be exactly enough. 
I'm using Yocto to build a kernel with some other patches(which have nothing 
to do with emmc). This is my base and on this base I test your patches. 
With your patches it is not booting and without it is booting. 

So, no ghosts at all.

Kind regards,
Benjamin Beckmeyer
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Jan. 5, 2018, 1:25 p.m. | #7
On Fri, Jan 5, 2018 at 12:08 PM, Benjamin Beckmeyer
<beckmeyer.b@rittal.de> wrote:
>> When you say you flashed "another kernel and it works" did you mean

>> some random kernel, or exactly the same you applied my patch on,

>> but without my patch?

>

>> Please confirm that you have a booting merge base so I'm not chasing

>> ghosts here :)

>

> Sorry for not be exactly enough.

> I'm using Yocto to build a kernel with some other patches(which have nothing

> to do with emmc). This is my base and on this base I test your patches.

> With your patches it is not booting and without it is booting.


OK we're back to guesswork, I guess maybe I need to use
dma_alloc_coherent() since the buffer gets bigger than 64K.

I'll test that and respin.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index e9290a3439d5..d35e25effe7d 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -502,8 +502,28 @@  static int sdhci_pre_dma_transfer(struct sdhci_host *host,
 	if (data->host_cookie == COOKIE_PRE_MAPPED)
 		return data->sg_count;
 
-	sg_count = dma_map_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
-			      mmc_get_dma_dir(data));
+	/* Bounce write requests to the bounce buffer */
+	if (host->bounce_buffer) {
+		if (mmc_get_dma_dir(data) == DMA_TO_DEVICE) {
+			pr_info("bouncing write data\n");
+
+			/* Copy the data to the bounce buffer */
+			sg_copy_to_buffer(data->sg, data->sg_len,
+					  host->bounce_buffer, host->bounce_buffer_size);
+		}
+
+		/* Map the bounce buffer to the device for both reads and writes */
+		sg_count = dma_map_sg(mmc_dev(host->mmc), host->bounce_sg,
+				      1, DMA_TO_DEVICE);
+
+		/* Overzealous check */
+		if (sg_count != 1)
+			pr_err("mapped bounce buffer more than one SG entry\n");
+	} else {
+		/* Just access the data directly from memory */
+		sg_count = dma_map_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
+				      mmc_get_dma_dir(data));
+	}
 
 	if (sg_count == 0)
 		return -ENOSPC;
@@ -858,8 +878,13 @@  static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 					     SDHCI_ADMA_ADDRESS_HI);
 		} else {
 			WARN_ON(sg_cnt != 1);
-			sdhci_writel(host, sg_dma_address(data->sg),
-				SDHCI_DMA_ADDRESS);
+			/* Bounce buffer goes to work */
+			if (host->bounce_buffer)
+				sdhci_writel(host, sg_dma_address(host->bounce_sg),
+					     SDHCI_DMA_ADDRESS);
+			else
+				sdhci_writel(host, sg_dma_address(data->sg),
+					     SDHCI_DMA_ADDRESS);
 		}
 	}
 
@@ -2248,7 +2273,12 @@  static void sdhci_pre_req(struct mmc_host *mmc, struct mmc_request *mrq)
 
 	mrq->data->host_cookie = COOKIE_UNMAPPED;
 
-	if (host->flags & SDHCI_REQ_USE_DMA)
+	/*
+	 * No pre-mapping in the pre hook if we're using the bounce buffer,
+	 * for that we would need two bounce buffers since one buffer is
+	 * in flight when this is getting called.
+	 */
+	if (host->flags & SDHCI_REQ_USE_DMA && !host->bounce_buffer)
 		sdhci_pre_dma_transfer(host, mrq->data, COOKIE_PRE_MAPPED);
 }
 
@@ -2352,8 +2382,25 @@  static bool sdhci_request_done(struct sdhci_host *host)
 		struct mmc_data *data = mrq->data;
 
 		if (data && data->host_cookie == COOKIE_MAPPED) {
-			dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
-				     mmc_get_dma_dir(data));
+			if (host->bounce_buffer) {
+				/* Unmap the bounce buffer */
+				dma_unmap_sg(mmc_dev(host->mmc), host->bounce_sg,
+					     1, mmc_get_dma_dir(data));
+
+				/* On reads, copy the data back to the sglist */
+				if (mmc_get_dma_dir(data) == DMA_FROM_DEVICE) {
+					pr_info("bouncing read data\n");
+
+					sg_copy_from_buffer(data->sg, data->sg_len,
+							    host->bounce_buffer,
+							    host->bounce_buffer_size);
+				}
+			} else {
+				/* Unmap the raw data */
+				dma_unmap_sg(mmc_dev(host->mmc), data->sg,
+					     data->sg_len,
+					     mmc_get_dma_dir(data));
+			}
 			data->host_cookie = COOKIE_UNMAPPED;
 		}
 	}
@@ -2636,7 +2683,12 @@  static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
 		 */
 		if (intmask & SDHCI_INT_DMA_END) {
 			u32 dmastart, dmanow;
-			dmastart = sg_dma_address(host->data->sg);
+
+			if (host->bounce_buffer)
+				dmastart = sg_dma_address(host->bounce_sg);
+			else
+				dmastart = sg_dma_address(host->data->sg);
+
 			dmanow = dmastart + host->data->bytes_xfered;
 			/*
 			 * Force update to the next DMA block boundary.
@@ -3713,6 +3765,50 @@  int sdhci_setup_host(struct sdhci_host *host)
 	 */
 	mmc->max_blk_count = (host->quirks & SDHCI_QUIRK_NO_MULTIBLOCK) ? 1 : 65535;
 
+	if (mmc->max_segs == 1) {
+		unsigned int max_blocks;
+		unsigned int max_seg_size;
+
+		max_seg_size = mmc->max_req_size;
+		max_blocks = max_seg_size / 512;
+
+		/*
+		 * When we just support one segment, we can get significant speedups
+		 * by the help of a bounce buffer to group scattered reads/writes
+		 * together.
+		 *
+		 * TODO: is this too big? Stealing too much memory? The old bounce
+		 * buffer is max 64K. This should be the 512K that SDMA can handle
+		 * if I read the code above right. Anyways let's try this.
+		 * FIXME: use devm_*
+		 */
+		host->bounce_buffer = kmalloc(max_seg_size, GFP_KERNEL);
+		if (!host->bounce_buffer) {
+			pr_err("failed to allocate %u bytes for bounce buffer\n",
+			       max_seg_size);
+			return -ENOMEM;
+		}
+		host->bounce_buffer_size = max_seg_size;
+
+		/*
+		 * Initialize single-entry sglist to point at the bounce buffer
+		 * FIXME: use devm_*
+		 */
+		host->bounce_sg = kmalloc(sizeof(*host->bounce_sg), GFP_KERNEL);
+		if (!host->bounce_sg) {
+			pr_err("failed to allocate bounce buffer sglist\n");
+			return -ENOMEM;
+		}
+		sg_init_one(host->bounce_sg, host->bounce_buffer, max_seg_size);
+
+		/* Lie about this since we're bouncing */
+		mmc->max_segs = max_blocks;
+		mmc->max_seg_size = max_seg_size;
+
+		pr_info("SDMA with bounce buffer: bounce up to %u segments into one, max segment size %u bytes\n",
+			max_blocks, max_seg_size);
+	}
+
 	return 0;
 
 unreg:
@@ -3743,6 +3839,12 @@  void sdhci_cleanup_host(struct sdhci_host *host)
 				  host->align_addr);
 	host->adma_table = NULL;
 	host->align_buffer = NULL;
+	/* kfree() on NULL is fine */
+	kfree(host->bounce_sg);
+	kfree(host->bounce_buffer);
+	/* Overzealous but following the style above */
+	host->bounce_sg = NULL;
+	host->bounce_buffer = NULL;
 }
 EXPORT_SYMBOL_GPL(sdhci_cleanup_host);
 
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 54bc444c317f..31fc2302454a 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -440,6 +440,9 @@  struct sdhci_host {
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
+	char *bounce_buffer;	/* For packing SDMA reads/writes */
+	size_t bounce_buffer_size;
+	struct scatterlist *bounce_sg;
 
 	const struct sdhci_ops *ops;	/* Low level hw interface */