mmc: sdhci: Support maximum DMA latency request via PM QoS

RFC PATCH In review Adrian Hunter 6 participants 17 messages Mar 24, 2015 – Jul 22, 2015

Subsystems (5)

MMC, SD and SDIO Suspend to RAM Hibernation Secure digital host controller interface (SDHCI) driver +1 more subsystems

lore

Share & tools

lore

Adrian Hunter

author maintainer

2015-03-24 13:40 UTC #0 lore raw

cover mmc: sdhci: Support maximum DMA latency request via PM QoS

Here are some patches to address an issue with SDHCI in Intel Baytrail. Intel Baytrail has been observed sometimes to hang if host controllers are using DMA while deep C-states are used. Workaround that by specifying a maximum DMA latency that will prevent deep C-states.

The first patch adds a new PM QOS function so that the SDHCI driver can do a "lazy" cancel of the QoS request from within its "finish" tasklet.

The second patch adds support to SDHCI for specifying a maximum DMA latency.

The third and fourth patches take that facility into use for Baytrail.

Ad hoc testing with Lenovo Thinkpad 10 showed a stress test could run for at least 24 hours with the patches, compared to less than an hour without.

These patches are on top of my driver strength patches which are on top of my re-tuning patches.

Adrian Hunter (4):
PM / QoS: Add pm_qos_cancel_request_lazy() that doesn't sleep
mmc: sdhci: Support maximum DMA latency request via PM QOS
mmc: sdhci-acpi: Fix device hang on Intel BayTrail
mmc: sdhci-pci: Fix device hang on Intel BayTrail

 drivers/mmc/host/sdhci-acpi.c | 32 ++++++++++++++++++++++++++
 drivers/mmc/host/sdhci-pci.c  | 13 +++++++++++
 drivers/mmc/host/sdhci.c      | 52 +++++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.h      |  7 ++++++
 include/linux/pm_qos.h        |  2 ++
 kernel/power/qos.c            | 20 +++++++++++++++++
 6 files changed, 126 insertions(+)

Regards
Adrian

Adrian Hunter

author maintainer

2015-03-24 13:40 UTC #1 lore raw

RFC PATCH 1/4 PM / QoS: Add pm_qos_cancel_request_lazy() that doesn't sleep

Add pm_qos_cancel_request_lazy() which is convenient for contexts that may not sleep.

Signed-off-by Adrian Hunter <[email protected]>

---
 include/linux/pm_qos.h |  2 ++
 kernel/power/qos.c     | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+)

include/linux/pm_qos.h +2 -0

index 7b3ae0c..f44d353 100644
@@ -126,6 +126,8 @@ void pm_qos_update_request(struct pm_qos_request *req,
 			   s32 new_value);
 void pm_qos_update_request_timeout(struct pm_qos_request *req,
 				   s32 new_value, unsigned long timeout_us);
+void pm_qos_cancel_request_lazy(struct pm_qos_request *req,
+				unsigned int timeout_us);
 void pm_qos_remove_request(struct pm_qos_request *req);
 
 int pm_qos_request(int pm_qos_class);

kernel/power/qos.c +20 -0

index 97b0df7..ac131cb 100644
@@ -517,6 +517,26 @@ void pm_qos_update_request_timeout(struct pm_qos_request *req, s32 new_value,
 }
 
 /**
+ * pm_qos_cancel_request_lazy - cancels an existing qos request lazily.
+ * @req : handle to list element holding a pm_qos request to use
+ * @timeout_us: the delay before cancelling this qos request in usecs.
+ *
+ * After timeout_us, this qos request is cancelled.
+ */
+void pm_qos_cancel_request_lazy(struct pm_qos_request *req,
+				unsigned int timeout_us)
+{
+	if (!req)
+		return;
+	if (WARN(!pm_qos_request_active(req),
+		 "%s called for unknown object.", __func__))
+		return;
+
+	schedule_delayed_work(&req->work, usecs_to_jiffies(timeout_us));
+}
+EXPORT_SYMBOL_GPL(pm_qos_cancel_request_lazy);
+
+/**
  * pm_qos_remove_request - modifies an existing qos request
  * @req: handle to request list element
  *

Signature · 1 line

1.9.1

Dov Levenglick

2015-04-20 14:00 UTC #2 lore raw

Re: [RFC PATCH 1/4] PM / QoS: Add pm_qos_cancel_request_lazy() that doesn't sleep

Quoted 14 lines

Add pm_qos_cancel_request_lazy() which is convenient for contexts that may not sleep.
Signed-off-by Adrian Hunter <[email protected]>
---
 include/linux/pm_qos.h |  2 ++
 kernel/power/qos.c     | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+)
include/linux/pm_qos.h +0 -0
index 7b3ae0c..f44d353 100644

include/linux/pm_qos.h void pm_qos_update_request(struct pm_qos_request *req, · line 133

Quoted 8 lines

include/linux/pm_qos.h +2 -0

@@ -126,6 +126,8 @@ void pm_qos_update_request(struct pm_qos_request *req,
 			   s32 new_value);
 void pm_qos_update_request_timeout(struct pm_qos_request *req,
 				   s32 new_value, unsigned long
 timeout_us);
+void pm_qos_cancel_request_lazy(struct pm_qos_request *req,
+				unsigned int timeout_us);
 void pm_qos_remove_request(struct pm_qos_request *req);

I think that this could be acheived using existing API if pm_qos_update_request_timeout() were to be called with the existing timeout value.
Since reading the existing timeout value is missing - and I think would be a useful feature to have for other use-cases - do you agree with such an approach?

Quoted 36 lines

int pm_qos_request(int pm_qos_class);

kernel/power/qos.c +20 -0

index 97b0df7..ac131cb 100644
@@ -517,6 +517,26 @@ void pm_qos_update_request_timeout(struct
 pm_qos_request *req, s32 new_value,
 }
 
 /**
+ * pm_qos_cancel_request_lazy - cancels an existing qos request lazily.
+ * @req : handle to list element holding a pm_qos request to use
+ * @timeout_us: the delay before cancelling this qos request in usecs.
+ *
+ * After timeout_us, this qos request is cancelled.
+ */
+void pm_qos_cancel_request_lazy(struct pm_qos_request *req,
+				unsigned int timeout_us)
+{
+	if (!req)
+		return;
+	if (WARN(!pm_qos_request_active(req),
+		 "%s called for unknown object.", __func__))
+		return;
+
+	schedule_delayed_work(&req->work, usecs_to_jiffies(timeout_us));
+}
+EXPORT_SYMBOL_GPL(pm_qos_cancel_request_lazy);
+
+/**
  * pm_qos_remove_request - modifies an existing qos request
  * @req: handle to request list element

*
--
1.9.1

Quoted 5 lines

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

QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Adrian Hunter

author maintainer

2015-04-21 08:26 UTC #3 lore raw

Re: [RFC PATCH 1/4] PM / QoS: Add pm_qos_cancel_request_lazy() that doesn't sleep

Quoted 18 lines · Dov Levenglick

Quoted 18 lines

Add pm_qos_cancel_request_lazy() which is convenient for contexts that may not sleep.

Signed-off-by Adrian Hunter <[email protected]>

---
 include/linux/pm_qos.h |  2 ++
 kernel/power/qos.c     | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+)

include/linux/pm_qos.h +0 -0

index 7b3ae0c..f44d353 100644
@@ -126,6 +126,8 @@ void pm_qos_update_request(struct pm_qos_request *req,
 			   s32 new_value);
 void pm_qos_update_request_timeout(struct pm_qos_request *req,
 				   s32 new_value, unsigned long
 timeout_us);

Quoted 8 lines

include/linux/pm_qos.h void pm_qos_update_request(struct pm_qos_request *req, · line 132
Quoted 4 lines
include/linux/pm_qos.h +2 -0
+void pm_qos_cancel_request_lazy(struct pm_qos_request *req,
+				unsigned int timeout_us);
 void pm_qos_remove_request(struct pm_qos_request *req);
 
I think that this could be acheived using existing API if pm_qos_update_request_timeout() were to be called with the existing timeout value.

I don't follow what you mean. There is no existing timeout value. Did you mean existing request value? There is still the difference wrt cancel_delayed_work_sync.

Quoted 53 lines

Since reading the existing timeout value is missing - and I think would be a useful feature to have for other use-cases - do you agree with such an approach?
Quoted 36 lines
int pm_qos_request(int pm_qos_class);
kernel/power/qos.c +20 -0
index 97b0df7..ac131cb 100644
@@ -517,6 +517,26 @@ void pm_qos_update_request_timeout(struct
 pm_qos_request *req, s32 new_value,
 }
 
 /**
+ * pm_qos_cancel_request_lazy - cancels an existing qos request lazily.
+ * @req : handle to list element holding a pm_qos request to use
+ * @timeout_us: the delay before cancelling this qos request in usecs.
+ *
+ * After timeout_us, this qos request is cancelled.
+ */
+void pm_qos_cancel_request_lazy(struct pm_qos_request *req,
+				unsigned int timeout_us)
+{
+	if (!req)
+		return;
+	if (WARN(!pm_qos_request_active(req),
+		 "%s called for unknown object.", __func__))
+		return;
+
+	schedule_delayed_work(&req->work, usecs_to_jiffies(timeout_us));
+}
+EXPORT_SYMBOL_GPL(pm_qos_cancel_request_lazy);
+
+/**
  * pm_qos_remove_request - modifies an existing qos request
  * @req: handle to request list element
*
--
1.9.1
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Dov Levenglick

2015-04-21 10:18 UTC #4 lore raw

Re: [RFC PATCH 1/4] PM / QoS: Add pm_qos_cancel_request_lazy() that doesn't sleep

Quoted 29 lines · Dov Levenglick

Quoted 9 lines
Quoted 9 lines
Add pm_qos_cancel_request_lazy() which is convenient for contexts that may not sleep.
Signed-off-by Adrian Hunter <[email protected]>
---
 include/linux/pm_qos.h |  2 ++
 kernel/power/qos.c     | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+)
Quoted 5 lines
include/linux/pm_qos.h void pm_qos_update_request(struct pm_qos_request
Quoted 5 lines
include/linux/pm_qos.h +0 -0
index 7b3ae0c..f44d353 100644
@@ -126,6 +126,8 @@ void pm_qos_update_request(struct pm_qos_request
*req,
Quoted 4 lines
Quoted 4 lines
include/linux/pm_qos.h +0 -0
 			   s32 new_value);
 void pm_qos_update_request_timeout(struct pm_qos_request *req,
 				   s32 new_value, unsigned long
 timeout_us);
Quoted 8 lines
include/linux/pm_qos.h void pm_qos_update_request(struct pm_qos_request *req, · line 132
Quoted 4 lines
include/linux/pm_qos.h +2 -0
+void pm_qos_cancel_request_lazy(struct pm_qos_request *req,
+				unsigned int timeout_us);
 void pm_qos_remove_request(struct pm_qos_request *req);
 
I think that this could be acheived using existing API if pm_qos_update_request_timeout() were to be called with the existing timeout value.

Quoted 3 lines

I don't follow what you mean. There is no existing timeout value. Did you mean existing request value? There is still the difference wrt cancel_delayed_work_sync.

I did mean the existing request value. Thanks.
There is also the cancel_delayed_work_sync, however I think that that should be called in any case in order to cancel any other pending timeout changes.

Quoted 58 lines

Quoted 2 lines
Since reading the existing timeout value is missing - and I think would
be
Quoted 12 lines
a useful feature to have for other use-cases - do you agree with such an approach?
Quoted 9 lines
int pm_qos_request(int pm_qos_class);
kernel/power/qos.c +0 -0
index 97b0df7..ac131cb 100644
@@ -517,6 +517,26 @@ void pm_qos_update_request_timeout(struct
 pm_qos_request *req, s32 new_value,
 }
 
Quoted 2 lines
kernel/power/qos.c void pm_qos_update_request_timeout(struct · line 521
Quoted 2 lines
kernel/power/qos.c +1 -0
 /**
+ * pm_qos_cancel_request_lazy - cancels an existing qos request
lazily.
Quoted 25 lines
Quoted 25 lines
kernel/power/qos.c +19 -0
+ * @req : handle to list element holding a pm_qos request to use
+ * @timeout_us: the delay before cancelling this qos request in usecs.
+ *
+ * After timeout_us, this qos request is cancelled.
+ */
+void pm_qos_cancel_request_lazy(struct pm_qos_request *req,
+				unsigned int timeout_us)
+{
+	if (!req)
+		return;
+	if (WARN(!pm_qos_request_active(req),
+		 "%s called for unknown object.", __func__))
+		return;
+
+	schedule_delayed_work(&req->work, usecs_to_jiffies(timeout_us));
+}
+EXPORT_SYMBOL_GPL(pm_qos_cancel_request_lazy);
+
+/**
  * pm_qos_remove_request - modifies an existing qos request
  * @req: handle to request list element
*
--
1.9.1
Quoted 9 lines
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
Quoted 4 lines
a Linux Foundation Collaborative Project

Quoted 5 lines

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

QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Adrian Hunter

author maintainer

2015-04-21 10:25 UTC #5 lore raw

Re: [RFC PATCH 1/4] PM / QoS: Add pm_qos_cancel_request_lazy() that doesn't sleep

Quoted 29 lines · Dov Levenglick

Quoted 29 lines · Dov Levenglick
Quoted 9 lines
Quoted 9 lines
Add pm_qos_cancel_request_lazy() which is convenient for contexts that may not sleep.
Signed-off-by Adrian Hunter <[email protected]>
---
 include/linux/pm_qos.h |  2 ++
 kernel/power/qos.c     | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+)
Quoted 5 lines
include/linux/pm_qos.h void pm_qos_update_request(struct pm_qos_request
Quoted 5 lines
include/linux/pm_qos.h +0 -0
index 7b3ae0c..f44d353 100644
@@ -126,6 +126,8 @@ void pm_qos_update_request(struct pm_qos_request
*req,
Quoted 12 lines
include/linux/pm_qos.h void pm_qos_update_request(struct pm_qos_request *req, · line 132
Quoted 8 lines
include/linux/pm_qos.h +2 -0
 			   s32 new_value);
 void pm_qos_update_request_timeout(struct pm_qos_request *req,
 				   s32 new_value, unsigned long
 timeout_us);
+void pm_qos_cancel_request_lazy(struct pm_qos_request *req,
+				unsigned int timeout_us);
 void pm_qos_remove_request(struct pm_qos_request *req);
 
I think that this could be acheived using existing API if pm_qos_update_request_timeout() were to be called with the existing timeout value.

Quoted 8 lines

Quoted 3 lines
I don't follow what you mean. There is no existing timeout value. Did you mean existing request value? There is still the difference wrt cancel_delayed_work_sync.
I did mean the existing request value. Thanks.
There is also the cancel_delayed_work_sync, however I think that that should be called in any case in order to cancel any other pending timeout changes.

That might sleep which defeats one of the reasons for creating pm_qos_cancel_request_lazy().

Quoted 72 lines

Quoted 59 lines
Quoted 3 lines
Since reading the existing timeout value is missing - and I think would
be
Quoted 12 lines
a useful feature to have for other use-cases - do you agree with such an approach?
Quoted 9 lines
int pm_qos_request(int pm_qos_class);
kernel/power/qos.c +0 -0
index 97b0df7..ac131cb 100644
@@ -517,6 +517,26 @@ void pm_qos_update_request_timeout(struct
 pm_qos_request *req, s32 new_value,
 }
 
Quoted 2 lines
kernel/power/qos.c void pm_qos_update_request_timeout(struct · line 521
Quoted 2 lines
kernel/power/qos.c +1 -0
 /**
+ * pm_qos_cancel_request_lazy - cancels an existing qos request
lazily.
Quoted 25 lines
Quoted 25 lines
kernel/power/qos.c +19 -0
+ * @req : handle to list element holding a pm_qos request to use
+ * @timeout_us: the delay before cancelling this qos request in usecs.
+ *
+ * After timeout_us, this qos request is cancelled.
+ */
+void pm_qos_cancel_request_lazy(struct pm_qos_request *req,
+				unsigned int timeout_us)
+{
+	if (!req)
+		return;
+	if (WARN(!pm_qos_request_active(req),
+		 "%s called for unknown object.", __func__))
+		return;
+
+	schedule_delayed_work(&req->work, usecs_to_jiffies(timeout_us));
+}
+EXPORT_SYMBOL_GPL(pm_qos_cancel_request_lazy);
+
+/**
  * pm_qos_remove_request - modifies an existing qos request
  * @req: handle to request list element
*
--
1.9.1
Quoted 9 lines
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
Quoted 4 lines
a Linux Foundation Collaborative Project
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Adrian Hunter

author maintainer

2015-03-24 13:40 UTC #6 lore raw

RFC PATCH 2/4 mmc: sdhci: Support maximum DMA latency request via PM QOS

In reply to Adrian Hunter

Add support for setting a maximum DMA latency via the PM QOS framework.

Drivers can set host->dma_latency to the desired value otherwise the initial value (PM_QOS_DEFAULT_VALUE) will result in no PM QOS request being added.

It may be that there isn't time between consecutive I/O requests to reach deeper C-states. To address that the driver can set host->lat_cancel_delay which is the delay before cancelling the DMA latency request when it is known that there is another request on the way.

Signed-off-by Adrian Hunter <[email protected]>

---
 drivers/mmc/host/sdhci.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.h |  7 +++++++
 2 files changed, 59 insertions(+)

drivers/mmc/host/sdhci.c +52 -0

index d8be0bf..3a0859b 100644
@@ -724,6 +724,45 @@ static void sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 	}
 }
 
+static bool sdhci_pm_qos_use_dma_latency(struct sdhci_host *host)
+{
+	return host->dma_latency != PM_QOS_DEFAULT_VALUE;
+}
+
+static void sdhci_pm_qos_set_dma_latency(struct sdhci_host *host,
+					 struct mmc_request *mrq)
+{
+	if (sdhci_pm_qos_use_dma_latency(host) && mrq->data &&
+	    (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA))) {
+		pm_qos_update_request(&host->pm_qos_req, host->dma_latency);
+		host->pm_qos_set = true;
+	}
+}
+
+static void sdhci_pm_qos_unset(struct sdhci_host *host)
+{
+	unsigned int delay;
+
+	if (host->pm_qos_set) {
+		host->pm_qos_set = false;
+		delay = host->consecutive_req ? host->lat_cancel_delay : 0;
+		pm_qos_cancel_request_lazy(&host->pm_qos_req, delay);
+	}
+}
+
+static void sdhci_pm_qos_add(struct sdhci_host *host)
+{
+	if (sdhci_pm_qos_use_dma_latency(host))
+		pm_qos_add_request(&host->pm_qos_req, PM_QOS_CPU_DMA_LATENCY,
+				   PM_QOS_DEFAULT_VALUE);
+}
+
+static void sdhci_pm_qos_remove(struct sdhci_host *host)
+{
+	if (pm_qos_request_active(&host->pm_qos_req))
+		pm_qos_remove_request(&host->pm_qos_req);
+}
+
 static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 {
 	u8 ctrl;

@@ -1348,6 +1387,8 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 
 	err = mmc_retune(mmc);
 
+	sdhci_pm_qos_set_dma_latency(host, mrq);
+
 	/* Firstly check card presence */
 	present = sdhci_do_get_cd(host);

@@ -1371,6 +1412,7 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 	}
 
 	host->mrq = mrq;
+	host->consecutive_req = 0;
 
 	if (!err && (!present || host->flags & SDHCI_DEVICE_DEAD))
 		err = -ENOMEDIUM;

@@ -2167,6 +2209,8 @@ static void sdhci_pre_req(struct mmc_host *mmc, struct mmc_request *mrq,
 {
 	struct sdhci_host *host = mmc_priv(mmc);
 
+	host->consecutive_req = 1;
+
 	if (mrq->data->host_cookie) {
 		mrq->data->host_cookie = 0;
 		return;

@@ -2243,6 +2287,8 @@ static void sdhci_tasklet_finish(unsigned long param)
 
 	host = (struct sdhci_host*)param;
 
+	sdhci_pm_qos_unset(host);
+
 	spin_lock_irqsave(&host->lock, flags);
 
         /*

@@ -2877,6 +2923,7 @@ struct sdhci_host *sdhci_alloc_host(struct device *dev,
 
 	host = mmc_priv(mmc);
 	host->mmc = mmc;
+	host->dma_latency = PM_QOS_DEFAULT_VALUE;
 
 	return host;
 }

@@ -3363,6 +3410,8 @@ int sdhci_add_host(struct sdhci_host *host)
 	 */
 	mmc->max_blk_count = (host->quirks & SDHCI_QUIRK_NO_MULTIBLOCK) ? 1 : 65535;
 
+	sdhci_pm_qos_add(host);
+
 	/*
 	 * Init tasklets.
 	 */

@@ -3426,6 +3475,7 @@ reset:
 #endif
 untasklet:
 	tasklet_kill(&host->finish_tasklet);
+	sdhci_pm_qos_remove(host);
 
 	return ret;
 }

@@ -3482,6 +3532,8 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
 
 	host->adma_table = NULL;
 	host->align_buffer = NULL;
+
+	sdhci_pm_qos_remove(host);
 }
 
 EXPORT_SYMBOL_GPL(sdhci_remove_host);

drivers/mmc/host/sdhci.h +7 -0

index 5521d29..15c5e7b 100644
@@ -19,6 +19,7 @@
 #include <linux/io.h>
 
 #include <linux/mmc/host.h>
+#include <linux/pm_qos.h>
 
 /*
  * Controller registers

@@ -419,6 +420,12 @@ struct sdhci_host {
 	struct mmc_host *mmc;	/* MMC structure */
 	u64 dma_mask;		/* custom DMA mask */
 
+	struct pm_qos_request pm_qos_req;
+	int dma_latency;
+	int lat_cancel_delay;
+	int consecutive_req;
+	bool pm_qos_set;
+
 #if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
 	struct led_classdev led;	/* LED control */
 	char led_name[32];

Signature · 1 line

1.9.1

Adrian Hunter

author maintainer

2015-03-24 13:40 UTC #7 lore raw

RFC PATCH 3/4 mmc: sdhci-acpi: Fix device hang on Intel BayTrail

In reply to Adrian Hunter

Intel Baytrail has been observed sometimes to hang if host controllers are using DMA while deep C-states are used. Workaround that by specifying a maximum DMA latency that will prevent deep C-states.

Unfortunately, host controller ACPI HIDs are not unique to Baytrail, so the CPU must be identified.

Signed-off-by Adrian Hunter <[email protected]>

---
 drivers/mmc/host/sdhci-acpi.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

drivers/mmc/host/sdhci-acpi.c +32 -0

index 22d929f..eaff09f 100644
@@ -43,6 +43,24 @@
 
 #include "sdhci.h"
 
+#ifdef CONFIG_X86
+#include <asm/cpu_device_id.h>
+static bool sdhci_acpi_on_byt(void)
+{
+	static const struct x86_cpu_id byt[] = {
+		{ X86_VENDOR_INTEL, 6, 0x37 },
+		{}
+	};
+
+	return x86_match_cpu(byt);
+}
+#else
+static bool sdhci_acpi_on_byt(void)
+{
+	return false;
+}
+#endif
+
 enum {
 	SDHCI_ACPI_SD_CD		= BIT(0),
 	SDHCI_ACPI_RUNTIME_PM		= BIT(1),

@@ -146,6 +164,14 @@ static const struct sdhci_acpi_chip sdhci_acpi_chip_int = {
 	.ops = &sdhci_acpi_ops_int,
 };
 
+static void sdhci_acpi_int_dma_latency(struct sdhci_host *host)
+{
+	if (sdhci_acpi_on_byt()) {
+		host->dma_latency = 20;
+		host->lat_cancel_delay = 275;
+	}
+}
+
 static int sdhci_acpi_emmc_probe_slot(struct platform_device *pdev,
 				      const char *hid, const char *uid)
 {

@@ -164,6 +190,8 @@ static int sdhci_acpi_emmc_probe_slot(struct platform_device *pdev,
 	    sdhci_readl(host, SDHCI_CAPABILITIES_1) == 0x00000807)
 		host->timeout_clk = 1000; /* 1000 kHz i.e. 1 MHz */
 
+	sdhci_acpi_int_dma_latency(host);
+
 	return 0;
 }

@@ -178,6 +206,8 @@ static int sdhci_acpi_sdio_probe_slot(struct platform_device *pdev,
 
 	host = c->host;
 
+	sdhci_acpi_int_dma_latency(host);
+
 	/* Platform specific code during sdio probe slot goes here */
 
 	return 0;

@@ -194,6 +224,8 @@ static int sdhci_acpi_sd_probe_slot(struct platform_device *pdev,
 
 	host = c->host;
 
+	sdhci_acpi_int_dma_latency(host);
+
 	/* Platform specific code during sd probe slot goes here */
 
 	return 0;

Signature · 1 line

1.9.1

Adrian Hunter

author maintainer

2015-03-24 13:40 UTC #8 lore raw

RFC PATCH 4/4 mmc: sdhci-pci: Fix device hang on Intel BayTrail

In reply to Adrian Hunter

Intel Baytrail has been observed sometimes to hang if host controllers are using DMA while deep C-states are used. Workaround that by specifying a maximum DMA latency that will prevent deep C-states.

Signed-off-by Adrian Hunter <[email protected]>

---
 drivers/mmc/host/sdhci-pci.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

drivers/mmc/host/sdhci-pci.c +13 -0

index 5a5b5e9..bba2956 100644
@@ -330,6 +330,12 @@ static void spt_read_drive_strength(struct sdhci_host *host)
 	sdhci_pci_spt_drive_strength = 0x10 | ((val >> 12) & 0xf);
 }
 
+static void byt_set_dma_latency(struct sdhci_host *host)
+{
+	host->dma_latency = 20;
+	host->lat_cancel_delay = 275;
+}
+
 static int byt_emmc_probe_slot(struct sdhci_pci_slot *slot)
 {
 	slot->host->mmc->caps |= MMC_CAP_8_BIT_DATA | MMC_CAP_NONREMOVABLE |

@@ -338,6 +344,9 @@ static int byt_emmc_probe_slot(struct sdhci_pci_slot *slot)
 				 MMC_CAP_WAIT_WHILE_BUSY;
 	slot->host->mmc->caps2 |= MMC_CAP2_HC_ERASE_SZ;
 	slot->hw_reset = sdhci_pci_int_hw_reset;
+	if (slot->chip->pdev->device == PCI_DEVICE_ID_INTEL_BYT_EMMC ||
+	    slot->chip->pdev->device == PCI_DEVICE_ID_INTEL_BYT_EMMC2)
+		byt_set_dma_latency(slot->host);
 	if (slot->chip->pdev->device == PCI_DEVICE_ID_INTEL_BSW_EMMC)
 		slot->host->timeout_clk = 1000; /* 1000 kHz i.e. 1 MHz */
 	if (slot->chip->pdev->device == PCI_DEVICE_ID_INTEL_SPT_EMMC) {

@@ -352,6 +361,8 @@ static int byt_sdio_probe_slot(struct sdhci_pci_slot *slot)
 	slot->host->mmc->caps |= MMC_CAP_POWER_OFF_CARD | MMC_CAP_NONREMOVABLE |
 				 MMC_CAP_BUS_WIDTH_TEST |
 				 MMC_CAP_WAIT_WHILE_BUSY;
+	if (slot->chip->pdev->device == PCI_DEVICE_ID_INTEL_BYT_SDIO)
+		byt_set_dma_latency(slot->host);
 	return 0;
 }

@@ -362,6 +373,8 @@ static int byt_sd_probe_slot(struct sdhci_pci_slot *slot)
 	slot->cd_con_id = NULL;
 	slot->cd_idx = 0;
 	slot->cd_override_level = true;
+	if (slot->chip->pdev->device == PCI_DEVICE_ID_INTEL_BYT_SD)
+		byt_set_dma_latency(slot->host);
 	return 0;
 }

Signature · 1 line

1.9.1

Rafael J. Wysocki

2015-03-24 19:48 UTC #9 lore raw

In reply to Adrian Hunter

Quoted 27 lines · Adrian Hunter

Hi
Here are some patches to address an issue with SDHCI in Intel Baytrail. Intel Baytrail has been observed sometimes to hang if host controllers are using DMA while deep C-states are used. Workaround that by specifying a maximum DMA latency that will prevent deep C-states.
The first patch adds a new PM QOS function so that the SDHCI driver can do a "lazy" cancel of the QoS request from within its "finish" tasklet.
The second patch adds support to SDHCI for specifying a maximum DMA latency.
The third and fourth patches take that facility into use for Baytrail.
Ad hoc testing with Lenovo Thinkpad 10 showed a stress test could run for at least 24 hours with the patches, compared to less than an hour without.
These patches are on top of my driver strength patches which are on top of my re-tuning patches.

Quoted 5 lines

Adrian Hunter (4):
PM / QoS: Add pm_qos_cancel_request_lazy() that doesn't sleep
mmc: sdhci: Support maximum DMA latency request via PM QOS
mmc: sdhci-acpi: Fix device hang on Intel BayTrail
mmc: sdhci-pci: Fix device hang on Intel BayTrail

I'm a bit concerned about the CPUID-based blacklisting of things and whether or not the SDHCI driver is the right place for doing that.

Maybe we should do it from within the LPSS driver instead?

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Adrian Hunter

author maintainer

2015-03-25 12:37 UTC #10 lore raw

Quoted 34 lines · Rafael J. Wysocki

Quoted 27 lines · Adrian Hunter
Hi
Here are some patches to address an issue with SDHCI in Intel Baytrail. Intel Baytrail has been observed sometimes to hang if host controllers are using DMA while deep C-states are used. Workaround that by specifying a maximum DMA latency that will prevent deep C-states.
The first patch adds a new PM QOS function so that the SDHCI driver can do a "lazy" cancel of the QoS request from within its "finish" tasklet.
The second patch adds support to SDHCI for specifying a maximum DMA latency.
The third and fourth patches take that facility into use for Baytrail.
Ad hoc testing with Lenovo Thinkpad 10 showed a stress test could run for at least 24 hours with the patches, compared to less than an hour without.
These patches are on top of my driver strength patches which are on top of my re-tuning patches.
Quoted 5 lines
Adrian Hunter (4):
PM / QoS: Add pm_qos_cancel_request_lazy() that doesn't sleep
mmc: sdhci: Support maximum DMA latency request via PM QOS
mmc: sdhci-acpi: Fix device hang on Intel BayTrail
mmc: sdhci-pci: Fix device hang on Intel BayTrail

Quoted 4 lines

I'm a bit concerned about the CPUID-based blacklisting of things and whether or not the SDHCI driver is the right place for doing that.
Maybe we should do it from within the LPSS driver instead?

+Mika

There are 2 minor difficulties with that:
1. The sdhci-acpi driver is not currently dependent on lpss
2. lpss does not currently export anything, so it is a bit of a new direction

If the ACPI HIDs used for SDHCI were unique to BayTrail (as the PCI device ids are) then there would be no need to consult the cpuid. So it seems to be a SDHCI problem, but I can't see a nice way to put it into lpss.

Pavel Machek

2015-03-25 19:43 UTC #11 lore raw

In reply to Adrian Hunter

Hi!

Quoted 4 lines

Here are some patches to address an issue with SDHCI in Intel Baytrail. Intel Baytrail has been observed sometimes to hang if host controllers are using DMA while deep C-states are used. Workaround that by

I wonder if there is more information on this one? I see your address is "@intel". For example which C states are affected, and if there's maybe Intel errata number to quote?
Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Adrian Hunter

author maintainer

2015-03-26 08:29 UTC #12 lore raw

Quoted 9 lines · Pavel Machek

Hi!
Quoted 4 lines
Here are some patches to address an issue with SDHCI in Intel Baytrail. Intel Baytrail has been observed sometimes to hang if host controllers are using DMA while deep C-states are used. Workaround that by
I wonder if there is more information on this one? I see your address is "@intel". For example which C states are affected, and if there's

The problem starts at C6 - I will add that to the commit message.

Quoted 1 line

maybe Intel errata number to quote?

I wasn't able to find an errata number.

Pavel Machek

2015-03-26 09:51 UTC #13 lore raw

Quoted 8 lines · Adrian Hunter

Quoted 7 lines · Pavel Machek
Hi!
Quoted 4 lines
Here are some patches to address an issue with SDHCI in Intel Baytrail. Intel Baytrail has been observed sometimes to hang if host controllers are using DMA while deep C-states are used. Workaround that by

Quoted 4 lines

Quoted 2 lines
I wonder if there is more information on this one? I see your address is "@intel". For example which C states are affected, and if there's
The problem starts at C6 - I will add that to the commit message.

Better find a place for a comment in code, too...

Thanks,
Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Len Brown

reviewer

2015-04-01 19:59 UTC #14 lore raw

In reply to Adrian Hunter

Quoted 3 lines

Ad hoc testing with Lenovo Thinkpad 10 showed a stress test could run for at least 24 hours with the patches, compared to less than an hour without.

There is a patch in linux-next to delete C1E from BYT,
since it is problematic on multiple platforms.
I don't suppose that just disabling that state without disabling C6 is sufficient to fix the Thinkpad 10? (I'm betting not, but it can't hurt to try -- you can use the "disable" attribute for the state in /sys/devices/system/cpu/cpu*/cpuidle/stateN)

I think your choice of the PM_QOS sub-system here is the right one, and that your selection of 20usec threshold is also a good choice for what you want to do -- though on non-intel_idle machine somplace, there may be some ACPI BIOS _CST with random number for C6 latency.

It would be interesting to see how your C6 residency (turbostat --debug will show this to you)
and your battery life are changed by disabling C6 during MMC activity.

cheers,
Len Brown, Intel Open Source Technology Center

Adrian Hunter

author maintainer

2015-04-02 19:35 UTC #15 lore raw

Quoted 11 lines · Len Brown

Quoted 3 lines
Ad hoc testing with Lenovo Thinkpad 10 showed a stress test could run for at least 24 hours with the patches, compared to less than an hour without.
There is a patch in linux-next to delete C1E from BYT,
since it is problematic on multiple platforms.
I don't suppose that just disabling that state without disabling C6 is sufficient to fix the Thinkpad 10? (I'm betting not, but it can't hurt to try -- you can use the "disable" attribute for the state in /sys/devices/system/cpu/cpu*/cpuidle/stateN)

Quoted 8 lines

I think your choice of the PM_QOS sub-system here is the right one, and that your selection of 20usec threshold is also a good choice for what you want to do -- though on non-intel_idle machine somplace, there may be some ACPI BIOS _CST with random number for C6 latency.
It would be interesting to see how your C6 residency (turbostat --debug will show this to you)
and your battery life are changed by disabling C6 during MMC activity.

I will do some more testing as you suggest, although it will have to wait until next week due to Easter holidays here.

Bastien Nocera

2015-07-22 12:04 UTC #16 lore raw

In reply to Adrian Hunter

Quoted 3 lines · Adrian Hunter <adrian.hunter <at> intel.com> writes:

Ad hoc testing with Lenovo Thinkpad 10 showed a stress test could run for at least 24 hours with the patches, compared to less than an hour without.

I've had the same sort of problems on 2 different BayTrail tablets (a Onda v975w and a WinBook TW100), usually hanging hard during package installation and was wondering whether you would resubmit your patches to be merged (I understand that the CPU detection was problematic), or whether I should ship those in our vendor kernel in the meanwhile.

I'll report shortly how well that patch series works after testing.

Quoted 2 lines

These patches are on top of my driver strength patches which are on top of my re-tuning patches.

Those 2 patchsets got merged upstream now, correct?

Cheers