* [PATCH V5 02/25] mmc: queue: Factor out mmc_queue_alloc_bounce_bufs()
From: Adrian Hunter @ 2016-10-24 8:37 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-mmc, Alex Lemberg, Mateusz Nowak, Yuliy Izrailov,
Jaehoon Chung, Dong Aisheng, Das Asutosh, Zhangfei Gao,
Dorfman Konstantin, David Griego, Sahitya Tummala, Harjani Ritesh,
Venu Byravarasu
In-Reply-To: <1477298260-5064-1-git-send-email-adrian.hunter@intel.com>
In preparation for supporting a queue of requests, factor out
mmc_queue_alloc_bounce_bufs().
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
drivers/mmc/card/queue.c | 45 +++++++++++++++++++++++++++------------------
1 file changed, 27 insertions(+), 18 deletions(-)
diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index af352b0410e1..f625dcd5db32 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -186,6 +186,31 @@ static void mmc_queue_setup_discard(struct request_queue *q,
queue_flag_set_unlocked(QUEUE_FLAG_SECERASE, q);
}
+static bool mmc_queue_alloc_bounce_bufs(struct mmc_queue *mq,
+ unsigned int bouncesz)
+{
+ struct mmc_queue_req *mqrq_cur = mq->mqrq_cur;
+ struct mmc_queue_req *mqrq_prev = mq->mqrq_prev;
+
+ mqrq_cur->bounce_buf = kmalloc(bouncesz, GFP_KERNEL);
+ if (!mqrq_cur->bounce_buf) {
+ pr_warn("%s: unable to allocate bounce cur buffer\n",
+ mmc_card_name(mq->card));
+ return false;
+ }
+
+ mqrq_prev->bounce_buf = kmalloc(bouncesz, GFP_KERNEL);
+ if (!mqrq_prev->bounce_buf) {
+ pr_warn("%s: unable to allocate bounce prev buffer\n",
+ mmc_card_name(mq->card));
+ kfree(mqrq_cur->bounce_buf);
+ mqrq_cur->bounce_buf = NULL;
+ return false;
+ }
+
+ return true;
+}
+
/**
* mmc_init_queue - initialise a queue structure.
* @mq: mmc queue
@@ -235,24 +260,8 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card,
if (bouncesz > (host->max_blk_count * 512))
bouncesz = host->max_blk_count * 512;
- if (bouncesz > 512) {
- mqrq_cur->bounce_buf = kmalloc(bouncesz, GFP_KERNEL);
- if (!mqrq_cur->bounce_buf) {
- pr_warn("%s: unable to allocate bounce cur buffer\n",
- mmc_card_name(card));
- } else {
- mqrq_prev->bounce_buf =
- kmalloc(bouncesz, GFP_KERNEL);
- if (!mqrq_prev->bounce_buf) {
- pr_warn("%s: unable to allocate bounce prev buffer\n",
- mmc_card_name(card));
- kfree(mqrq_cur->bounce_buf);
- mqrq_cur->bounce_buf = NULL;
- }
- }
- }
-
- if (mqrq_cur->bounce_buf && mqrq_prev->bounce_buf) {
+ if (bouncesz > 512 &&
+ mmc_queue_alloc_bounce_bufs(mq, bouncesz)) {
blk_queue_bounce_limit(mq->queue, BLK_BOUNCE_ANY);
blk_queue_max_hw_sectors(mq->queue, bouncesz / 512);
blk_queue_max_segments(mq->queue, bouncesz / 512);
--
1.9.1
^ permalink raw reply related
* [PATCH V5 01/25] mmc: queue: Fix queue thread wake-up
From: Adrian Hunter @ 2016-10-24 8:37 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-mmc, Alex Lemberg, Mateusz Nowak, Yuliy Izrailov,
Jaehoon Chung, Dong Aisheng, Das Asutosh, Zhangfei Gao,
Dorfman Konstantin, David Griego, Sahitya Tummala, Harjani Ritesh,
Venu Byravarasu
In-Reply-To: <1477298260-5064-1-git-send-email-adrian.hunter@intel.com>
The only time the driver sleeps expecting to be woken upon the arrival of
a new request, is when the dispatch queue is empty. The only time that it
is known whether the dispatch queue is empty is after NULL is returned
from blk_fetch_request() while under the queue lock.
Recognizing those facts, simplify the synchronization between the queue
thread and the request function. A couple of flags tell the request
function what to do, and the queue lock and barriers associated with
wake-ups ensure synchronization.
The result is simpler and allows the removal of the context_info lock.
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
drivers/mmc/card/block.c | 7 -------
drivers/mmc/card/queue.c | 35 +++++++++++++++++++++--------------
drivers/mmc/card/queue.h | 1 +
drivers/mmc/core/core.c | 6 ------
include/linux/mmc/host.h | 2 --
5 files changed, 22 insertions(+), 29 deletions(-)
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 709a872ed484..485fba5ab6c0 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -2150,8 +2150,6 @@ int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
int ret;
struct mmc_blk_data *md = mq->data;
struct mmc_card *card = md->queue.card;
- struct mmc_host *host = card->host;
- unsigned long flags;
bool req_is_special = mmc_req_is_special(req);
if (req && !mq->mqrq_prev->req)
@@ -2184,11 +2182,6 @@ int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
mmc_blk_issue_rw_rq(mq, NULL);
ret = mmc_blk_issue_flush(mq, req);
} else {
- if (!req && host->areq) {
- spin_lock_irqsave(&host->context_info.lock, flags);
- host->context_info.is_waiting_last_req = true;
- spin_unlock_irqrestore(&host->context_info.lock, flags);
- }
ret = mmc_blk_issue_rw_rq(mq, req);
}
diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index 8037f73a109a..af352b0410e1 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -53,6 +53,7 @@ static int mmc_queue_thread(void *d)
{
struct mmc_queue *mq = d;
struct request_queue *q = mq->queue;
+ struct mmc_context_info *cntx = &mq->card->host->context_info;
current->flags |= PF_MEMALLOC;
@@ -63,6 +64,19 @@ static int mmc_queue_thread(void *d)
spin_lock_irq(q->queue_lock);
set_current_state(TASK_INTERRUPTIBLE);
req = blk_fetch_request(q);
+ mq->asleep = false;
+ cntx->is_waiting_last_req = false;
+ cntx->is_new_req = false;
+ if (!req) {
+ /*
+ * Dispatch queue is empty so set flags for
+ * mmc_request_fn() to wake us up.
+ */
+ if (mq->mqrq_prev->req)
+ cntx->is_waiting_last_req = true;
+ else
+ mq->asleep = true;
+ }
mq->mqrq_cur->req = req;
spin_unlock_irq(q->queue_lock);
@@ -115,7 +129,6 @@ static void mmc_request_fn(struct request_queue *q)
{
struct mmc_queue *mq = q->queuedata;
struct request *req;
- unsigned long flags;
struct mmc_context_info *cntx;
if (!mq) {
@@ -127,19 +140,13 @@ static void mmc_request_fn(struct request_queue *q)
}
cntx = &mq->card->host->context_info;
- if (!mq->mqrq_cur->req && mq->mqrq_prev->req) {
- /*
- * New MMC request arrived when MMC thread may be
- * blocked on the previous request to be complete
- * with no current request fetched
- */
- spin_lock_irqsave(&cntx->lock, flags);
- if (cntx->is_waiting_last_req) {
- cntx->is_new_req = true;
- wake_up_interruptible(&cntx->wait);
- }
- spin_unlock_irqrestore(&cntx->lock, flags);
- } else if (!mq->mqrq_cur->req && !mq->mqrq_prev->req)
+
+ if (cntx->is_waiting_last_req) {
+ cntx->is_new_req = true;
+ wake_up_interruptible(&cntx->wait);
+ }
+
+ if (mq->asleep)
wake_up_process(mq->thread);
}
diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h
index 342f1e3f301e..73d70c5cee40 100644
--- a/drivers/mmc/card/queue.h
+++ b/drivers/mmc/card/queue.h
@@ -57,6 +57,7 @@ struct mmc_queue {
unsigned int flags;
#define MMC_QUEUE_SUSPENDED (1 << 0)
#define MMC_QUEUE_NEW_REQUEST (1 << 1)
+ bool asleep;
void *data;
struct request_queue *queue;
struct mmc_queue_req mqrq[2];
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 2553d903a82b..36b727feecac 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -504,18 +504,14 @@ static int mmc_wait_for_data_req_done(struct mmc_host *host,
struct mmc_command *cmd;
struct mmc_context_info *context_info = &host->context_info;
int err;
- unsigned long flags;
while (1) {
wait_event_interruptible(context_info->wait,
(context_info->is_done_rcv ||
context_info->is_new_req));
- spin_lock_irqsave(&context_info->lock, flags);
context_info->is_waiting_last_req = false;
- spin_unlock_irqrestore(&context_info->lock, flags);
if (context_info->is_done_rcv) {
context_info->is_done_rcv = false;
- context_info->is_new_req = false;
cmd = mrq->cmd;
if (!cmd->error || !cmd->retries ||
@@ -534,7 +530,6 @@ static int mmc_wait_for_data_req_done(struct mmc_host *host,
continue; /* wait for done/new event again */
}
} else if (context_info->is_new_req) {
- context_info->is_new_req = false;
if (!next_req)
return MMC_BLK_NEW_REQUEST;
}
@@ -3019,7 +3014,6 @@ void mmc_unregister_pm_notifier(struct mmc_host *host)
*/
void mmc_init_context_info(struct mmc_host *host)
{
- spin_lock_init(&host->context_info.lock);
host->context_info.is_new_req = false;
host->context_info.is_done_rcv = false;
host->context_info.is_waiting_last_req = false;
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 0b2439441cc8..3386e10480f7 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -198,14 +198,12 @@ struct mmc_slot {
* @is_new_req wake up reason was new request
* @is_waiting_last_req mmc context waiting for single running request
* @wait wait queue
- * @lock lock to protect data fields
*/
struct mmc_context_info {
bool is_done_rcv;
bool is_new_req;
bool is_waiting_last_req;
wait_queue_head_t wait;
- spinlock_t lock;
};
struct regulator;
--
1.9.1
^ permalink raw reply related
* [PATCH V5 00/25] mmc: mmc: Add Software Command Queuing
From: Adrian Hunter @ 2016-10-24 8:37 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-mmc, Alex Lemberg, Mateusz Nowak, Yuliy Izrailov,
Jaehoon Chung, Dong Aisheng, Das Asutosh, Zhangfei Gao,
Dorfman Konstantin, David Griego, Sahitya Tummala, Harjani Ritesh,
Venu Byravarasu
Hi
Here is an updated version of the Software Command Queuing patches,
re-based on next, with patches 1-5 dropped because they have been applied,
and 2 fixes that have been rolled in (refer Changes in V5 below).
Performance results:
Results can vary from run to run, but here are some results showing 1, 2 or 4
processes with 4k and 32k record sizes. They show up to 40% improvement in
read performance when there are multiple processes.
iozone -s 8192k -r 4k -i 0 -i 1 -i 2 -i 8 -I -t 1 -F /mnt/mmc/iozone1.tmp
Children see throughput for 1 initial writers = 27909.87 kB/sec 24204.14 kB/sec -13.28 %
Children see throughput for 1 rewriters = 28839.28 kB/sec 25531.92 kB/sec -11.47 %
Children see throughput for 1 readers = 25889.65 kB/sec 24883.23 kB/sec -3.89 %
Children see throughput for 1 re-readers = 25558.23 kB/sec 24679.89 kB/sec -3.44 %
Children see throughput for 1 random readers = 25571.48 kB/sec 24689.52 kB/sec -3.45 %
Children see throughput for 1 mixed workload = 25758.59 kB/sec 24487.52 kB/sec -4.93 %
Children see throughput for 1 random writers = 24787.51 kB/sec 19368.99 kB/sec -21.86 %
iozone -s 8192k -r 32k -i 0 -i 1 -i 2 -i 8 -I -t 1 -F /mnt/mmc/iozone1.tmp
Children see throughput for 1 initial writers = 91344.61 kB/sec 102008.56 kB/sec 11.67 %
Children see throughput for 1 rewriters = 87932.36 kB/sec 96630.44 kB/sec 9.89 %
Children see throughput for 1 readers = 134879.82 kB/sec 110292.79 kB/sec -18.23 %
Children see throughput for 1 re-readers = 147632.13 kB/sec 109053.33 kB/sec -26.13 %
Children see throughput for 1 random readers = 93547.37 kB/sec 112225.50 kB/sec 19.97 %
Children see throughput for 1 mixed workload = 93560.04 kB/sec 110515.21 kB/sec 18.12 %
Children see throughput for 1 random writers = 92841.84 kB/sec 81153.81 kB/sec -12.59 %
iozone -s 8192k -r 4k -i 0 -i 1 -i 2 -i 8 -I -t 2 -F /mnt/mmc/iozone1.tmp /mnt/mmc/iozone2.tmp
Children see throughput for 2 initial writers = 31145.43 kB/sec 33771.25 kB/sec 8.43 %
Children see throughput for 2 rewriters = 30592.57 kB/sec 35916.46 kB/sec 17.40 %
Children see throughput for 2 readers = 31669.83 kB/sec 37460.13 kB/sec 18.28 %
Children see throughput for 2 re-readers = 32079.94 kB/sec 37373.33 kB/sec 16.50 %
Children see throughput for 2 random readers = 27731.19 kB/sec 37601.65 kB/sec 35.59 %
Children see throughput for 2 mixed workload = 13927.50 kB/sec 14617.06 kB/sec 4.95 %
Children see throughput for 2 random writers = 31250.00 kB/sec 33106.72 kB/sec 5.94 %
iozone -s 8192k -r 32k -i 0 -i 1 -i 2 -i 8 -I -t 2 -F /mnt/mmc/iozone1.tmp /mnt/mmc/iozone2.tmp
Children see throughput for 2 initial writers = 123255.84 kB/sec 131252.22 kB/sec 6.49 %
Children see throughput for 2 rewriters = 115234.91 kB/sec 107225.74 kB/sec -6.95 %
Children see throughput for 2 readers = 128921.86 kB/sec 148562.71 kB/sec 15.23 %
Children see throughput for 2 re-readers = 127815.24 kB/sec 149304.32 kB/sec 16.81 %
Children see throughput for 2 random readers = 125600.46 kB/sec 148406.56 kB/sec 18.16 %
Children see throughput for 2 mixed workload = 44006.94 kB/sec 50937.36 kB/sec 15.75 %
Children see throughput for 2 random writers = 120623.95 kB/sec 103969.05 kB/sec -13.81 %
iozone -s 8192k -r 4k -i 0 -i 1 -i 2 -i 8 -I -t 4 -F /mnt/mmc/iozone1.tmp /mnt/mmc/iozone2.tmp /mnt/mmc/iozone3.tmp /mnt/mmc/iozone4.tmp
Children see throughput for 4 initial writers = 24100.96 kB/sec 33336.58 kB/sec 38.32 %
Children see throughput for 4 rewriters = 31650.20 kB/sec 33091.53 kB/sec 4.55 %
Children see throughput for 4 readers = 33276.92 kB/sec 41799.89 kB/sec 25.61 %
Children see throughput for 4 re-readers = 31786.96 kB/sec 41501.74 kB/sec 30.56 %
Children see throughput for 4 random readers = 31991.65 kB/sec 40973.93 kB/sec 28.08 %
Children see throughput for 4 mixed workload = 15804.80 kB/sec 13581.32 kB/sec -14.07 %
Children see throughput for 4 random writers = 31231.42 kB/sec 34537.03 kB/sec 10.58 %
iozone -s 8192k -r 32k -i 0 -i 1 -i 2 -i 8 -I -t 4 -F /mnt/mmc/iozone1.tmp /mnt/mmc/iozone2.tmp /mnt/mmc/iozone3.tmp /mnt/mmc/iozone4.tmp
Children see throughput for 4 initial writers = 116567.42 kB/sec 119280.35 kB/sec 2.33 %
Children see throughput for 4 rewriters = 115010.96 kB/sec 120864.34 kB/sec 5.09 %
Children see throughput for 4 readers = 130700.29 kB/sec 177834.21 kB/sec 36.06 %
Children see throughput for 4 re-readers = 125392.58 kB/sec 175975.28 kB/sec 40.34 %
Children see throughput for 4 random readers = 132194.57 kB/sec 176630.46 kB/sec 33.61 %
Children see throughput for 4 mixed workload = 56464.98 kB/sec 54140.61 kB/sec -4.12 %
Children see throughput for 4 random writers = 109128.36 kB/sec 85359.80 kB/sec -21.78 %
The current block driver supports 2 requests on the go at a time. Patches
1 - 8 make preparations for an arbitrary sized queue. Patches 9 - 12
introduce Command Queue definitions and helpers. Patches 13 - 19
complete the job of making the block driver use a queue. Patches 20 - 23
finally add Software Command Queuing, and 24 - 25 enable it for Intel eMMC
controllers. Most of the Software Command Queuing functionality is added
in patch 22.
As noted below, the patches can also be found here:
http://git.infradead.org/users/ahunter/linux-sdhci.git/shortlog/refs/heads/swcmdq
Changes in V5:
Patches 1-5 dropped because they have been applied.
Re-based on next.
Fixed use of blk_end_request_cur() when it should have been
blk_end_request_all() to error out requests during error recovery.
Fixed unpaired retune_hold / retune_release in the error recovery path.
Changes in V4:
Re-based on next + v4.8-rc2 + "block: Fix secure erase" patch
Changes in V3:
Patches 1-25 dropped because they have been applied.
Re-based on next.
mmc: queue: Allocate queue of size qdepth
Free queue during cleanup
mmc: mmc: Add Command Queue definitions
Add cmdq_en to mmc-dev-attrs.txt documentation
mmc: queue: Share mmc request array between partitions
New patch
Changes in V2:
Added 5 patches already sent here:
http://marc.info/?l=linux-mmc&m=146712062816835
Added 3 more new patches:
mmc: sdhci-pci: Do not runtime suspend at the end of sdhci_pci_probe()
mmc: sdhci: Avoid STOP cmd triggering warning in sdhci_send_command()
mmc: sdhci: sdhci_execute_tuning() must delete timer
Carried forward the V2 fix to:
mmc: mmc_test: Disable Command Queue while mmc_test is used
Also reset the cmd circuit for data timeout if it is processing the data
cmd, in patch:
mmc: sdhci: Do not reset cmd or data circuits that are in use
There wasn't much comment on the RFC so there have been few changes.
Venu Byravarasu commented that it may be more efficient to use Software
Command Queuing only when there is more than 1 request queued - it isn't
obvious how well that would work in practice, but it could be added later
if it could be shown to be beneficial.
Original Cover Letter:
Chuanxiao Dong sent some patches last year relating to eMMC 5.1 Software
Command Queuing. He did not follow-up but I have contacted him and he says
it is OK if I take over upstreaming the patches.
eMMC Command Queuing is a feature added in version 5.1. The card maintains
a queue of up to 32 data transfers. Commands CMD45/CMD45 are sent to queue
up transfers in advance, and then one of the transfers is selected to
"execute" by CMD46/CMD47 at which point data transfer actually begins.
The advantage of command queuing is that the card can prepare for transfers
in advance. That makes a big difference in the case of random reads because
the card can start reading into its cache in advance.
A v5.1 host controller can manage the command queue itself, but it is also
possible for software to manage the queue using an non-v5.1 host controller
- that is what Software Command Queuing is.
Refer to the JEDEC (http://www.jedec.org/) eMMC v5.1 Specification for more
information about Command Queuing.
While these patches are heavily based on Dong's patches, there are some
changes:
SDHCI has been amended to support commands during transfer. That is a
generic change added in patches 1 - 5. [Those patches have now been applied]
In principle, that would also support SDIO's CMD52 during data transfer.
The original approach added multiple commands into the same request for
sending CMD44, CMD45 and CMD13. That is not strictly necessary and has
been omitted for now.
The original approach also called blk_end_request() from the mrq->done()
function, which means the upper layers learnt of completed requests
slightly earlier. That is not strictly related to Software Command Queuing
and is something that could potentially be done for all data requests.
That has been omitted for now.
The current block driver supports 2 requests on the go at a time. Patches
1 - 8 make preparations for an arbitrary sized queue. Patches 9 - 12
introduce Command Queue definitions and helpers. Patches 13 - 19
complete the job of making the block driver use a queue. Patches 20 - 23
finally add Software Command Queuing, and 24 - 25 enable it for Intel eMMC
controllers. Most of the Software Command Queuing functionality is added
in patch 22.
The patches can also be found here:
http://git.infradead.org/users/ahunter/linux-sdhci.git/shortlog/refs/heads/swcmdq
The patches have only had basic testing so far. Ad-hoc testing shows a
degradation in sequential read performance of about 10% but an increase in
throughput for mixed workload of multiple processes of about 90%. The
reduction in sequential performance is due to the need to read the Queue
Status register between each transfer.
These patches should not conflict with Hardware Command Queuing which
handles the queue in a completely different way and thus does not need
to share code with Software Command Queuing. The exceptions being the
Command Queue definitions and queue allocation which should be able to be
used.
Adrian Hunter (25):
mmc: queue: Fix queue thread wake-up
mmc: queue: Factor out mmc_queue_alloc_bounce_bufs()
mmc: queue: Factor out mmc_queue_alloc_bounce_sgs()
mmc: queue: Factor out mmc_queue_alloc_sgs()
mmc: queue: Factor out mmc_queue_reqs_free_bufs()
mmc: queue: Introduce queue depth
mmc: queue: Use queue depth to allocate and free
mmc: queue: Allocate queue of size qdepth
mmc: mmc: Add Command Queue definitions
mmc: mmc: Add functions to enable / disable the Command Queue
mmc: mmc_test: Disable Command Queue while mmc_test is used
mmc: block: Disable Command Queue while RPMB is used
mmc: core: Do not prepare a new request twice
mmc: core: Export mmc_retune_hold() and mmc_retune_release()
mmc: block: Factor out mmc_blk_requeue()
mmc: block: Fix 4K native sector check
mmc: block: Use local var for mqrq_cur
mmc: block: Pass mqrq to mmc_blk_prep_packed_list()
mmc: block: Introduce queue semantics
mmc: queue: Share mmc request array between partitions
mmc: queue: Add a function to control wake-up on new requests
mmc: block: Add Software Command Queuing
mmc: mmc: Enable Software Command Queuing
mmc: sdhci-pci: Enable Software Command Queuing for some Intel controllers
mmc: sdhci-acpi: Enable Software Command Queuing for some Intel controllers
Documentation/mmc/mmc-dev-attrs.txt | 1 +
drivers/mmc/card/block.c | 738 +++++++++++++++++++++++++++++++++---
drivers/mmc/card/mmc_test.c | 13 +
drivers/mmc/card/queue.c | 332 ++++++++++------
drivers/mmc/card/queue.h | 35 +-
drivers/mmc/core/core.c | 18 +-
drivers/mmc/core/host.c | 2 +
drivers/mmc/core/host.h | 2 -
drivers/mmc/core/mmc.c | 43 ++-
drivers/mmc/core/mmc_ops.c | 27 ++
drivers/mmc/host/sdhci-acpi.c | 2 +-
drivers/mmc/host/sdhci-pci-core.c | 2 +-
include/linux/mmc/card.h | 9 +
include/linux/mmc/core.h | 5 +
include/linux/mmc/host.h | 4 +-
include/linux/mmc/mmc.h | 17 +
16 files changed, 1046 insertions(+), 204 deletions(-)
Regards
Adrian
^ permalink raw reply
* Re: [PATCH 4/4] mmc: core: Don't use ->card_busy() and CMD13 in combination when polling
From: Linus Walleij @ 2016-10-24 7:49 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-mmc@vger.kernel.org, Jaehoon Chung, Adrian Hunter,
Chaotian Jing
In-Reply-To: <1476951579-26125-5-git-send-email-ulf.hansson@linaro.org>
On Thu, Oct 20, 2016 at 10:19 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> When polling for busy after sending a MMC_SWITCH command, both the optional
> ->card_busy() callback and CMD13 are being used in conjunction.
>
> This doesn't make sense. Instead it's more reasonable to rely solely on the
> ->card_busy() callback when it exists. Let's change that and instead use
> the CMD13 as a fall-back. In this way we avoid sending CMD13, unless it's
> really needed.
>
> Within this context, let's also take the opportunity to make some
> additional clean-ups and clarifications to the related code.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Tried these four patches but it doesn't make my root mount before
20 minutes on the Dragonboard, but I don't know if that was the
intention even? Just cleanup? Sorry if I got it wrong...
Yours,
Linus Walleij
^ permalink raw reply
* Re: [RFC v2 2/2] mmc: sdhci: Ignore capability register when it comes to speeds and use DT binding instead when sdhci-cap-speed-modes-broken is set.
From: Adrian Hunter @ 2016-10-24 7:34 UTC (permalink / raw)
To: Zach Brown, ulf.hansson
Cc: robh+dt, mark.rutland, linux-mmc, devicetree, linux-kernel
In-Reply-To: <1477085715-30213-3-git-send-email-zach.brown@ni.com>
On 22/10/16 00:35, Zach Brown wrote:
> When the sdhci-cap-speed-modes-broken DT property is set, the driver
> will ignore the bits of the capability registers that correspond to
> speed modes.
>
> Signed-off-by: Zach Brown <zach.brown@ni.com>
> ---
> drivers/mmc/host/sdhci.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 1e25b01..59c62d3 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -22,6 +22,7 @@
> #include <linux/scatterlist.h>
> #include <linux/regulator/consumer.h>
> #include <linux/pm_runtime.h>
> +#include <linux/of.h>
>
> #include <linux/leds.h>
>
> @@ -3013,10 +3014,19 @@ void __sdhci_read_caps(struct sdhci_host *host, u16 *ver, u32 *caps, u32 *caps1)
>
> host->caps = caps ? *caps : sdhci_readl(host, SDHCI_CAPABILITIES);
>
> + if (of_property_read_bool(mmc_dev(host->mmc)->of_node,
> + "sdhci-cap-speed-modes-broken"))
It rather begs the question: if you are going to do something sdhci
specific, why not just read the whole of the caps register from DT?
> + host->caps &= ~SDHCI_CAN_DO_HISPD;
> +
> if (host->version < SDHCI_SPEC_300)
> return;
>
> host->caps1 = caps1 ? *caps1 : sdhci_readl(host, SDHCI_CAPABILITIES_1);
> +
> + if (of_property_read_bool(mmc_dev(host->mmc)->of_node,
> + "sdhci-cap-speed-modes-broken"))
Why read it twice?
> + host->caps1 &= ~(SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_SDR104 |
> + SDHCI_SUPPORT_DDR50 | SDHCI_SUPPORT_HS400);
> }
> EXPORT_SYMBOL_GPL(__sdhci_read_caps);
>
>
^ permalink raw reply
* [GIT PULL] Update dw-mmc controller
From: Jaehoon Chung @ 2016-10-23 22:52 UTC (permalink / raw)
To: linux-mmc@vger.kernel.org; +Cc: Ulf Hansson, Shawn Lin
Dear Ulf,
Could you pull these patches into your mmc-next?
The following changes since commit 2a688788ba1dd1dc905c24bf5b617aefdff4a68b:
mmc: core: Don't use ->card_busy() and CMD13 in combination when polling (2016-10-21 10:18:07 +0200)
are available in the git repository at:
https://github.com/jh80chung/dw-mmc.git for-ulf
for you to fetch changes up to 0a9bd53611e862d2a5e2f68513b2677b13b7b379:
mmc: dw_mmc: remove system PM callback (2016-10-24 07:13:07 +0900)
----------------------------------------------------------------
Shawn Lin (9):
mmc: dw_mmc: add runtime PM callback
mmc: dw_mmc-rockchip: add runtime PM support
mmc: core: expose the capability of gpio card detect
mmc: dw_mmc: disable biu clk if possible
mmc: dw_mmc-k3: deploy runtime PM facilities
mmc: dw_mmc-exynos: deploy runtime PM facilities
mmc: dw_mmc-pci: deploy runtime PM facilities
mmc: dw_mmc-pltfm: deploy runtime PM facilities
mmc: dw_mmc: remove system PM callback
drivers/mmc/core/slot-gpio.c | 8 +++++++
drivers/mmc/host/dw_mmc-exynos.c | 24 +++++++++-----------
drivers/mmc/host/dw_mmc-k3.c | 39 ++++++++------------------------
drivers/mmc/host/dw_mmc-pci.c | 29 ++++++++----------------
drivers/mmc/host/dw_mmc-pltfm.c | 28 +++++++----------------
drivers/mmc/host/dw_mmc-rockchip.c | 42 +++++++++++++++++++++++++++++++---
drivers/mmc/host/dw_mmc.c | 46 +++++++++++++++++++++++++-------------
drivers/mmc/host/dw_mmc.h | 6 ++---
include/linux/mmc/slot-gpio.h | 1 +
9 files changed, 117 insertions(+), 106 deletions(-)
Best Regards,
Jaehoon Chung
^ permalink raw reply
* Help: Reading CID and CSD from MMC/SD card
From: Natalia Portillo @ 2016-10-22 20:47 UTC (permalink / raw)
To: linux-mmc
Hi all,
Sorry if this mailing list is not the correct place to ask for help but
I've been unable to find another place.
I'm trying to read CID and CSD from MMC/SD cards using ioctl (reading
from /sys is not a solution for me).
>From what I've seen I should send a CMD7 to move card to standby, then
CMD9/10 and then CMD7 to move card to transition state.
While first command seems to work, next ones get error -110, and the
card stays standby until physical ejection/reset.
After several tries I have not found the cause of this problem.
I have pasted snippet of code I'm using at the end of this email.
Test machine is QEMU with kernel 3.16.36-1+deb8u1, amd64 and QEMU's
virtual SDHCI-PCI device.
Regards,
Natalia Portillo
--------------- CODE FOLLOWS -----------------------------
#include <linux/ioctl.h>
#include <stdio.h>
#include <sys/types.h>
#include <fcntl.h>
#include <sys/stat.h>
#include <stdlib.h>
#include <linux/mmc/ioctl.h>
#include <string.h>
#include <sys/ioctl.h>
#define MMC_RSP_PRESENT (1 << 0)
#define MMC_RSP_CRC (1 << 2)
#define MMC_RSP_OPCODE (1 << 4)
#define MMC_RSP_R1 (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE)
int main(int argc, char *argv)
{
int fd, fw;
void *buffer;
int address = 0;
int ret = 0;
struct mmc_ioc_cmd idata;
fw = open("cid.bin", O_CREAT | O_RDWR);
fd = open("/dev/mmcblk0", O_RDONLY);
if(fd < 0)
{
perror("file open");
return fd;
}
buffer = malloc(16);
memset(&idata, 0, sizeof(idata));
memset(buffer, 0, 16);
// Set standby
idata.opcode = 7;
idata.arg = 0;
idata.flags = 0x15;
idata.blksz = 0;
idata.blocks = 0;
mmc_ioc_cmd_set_data(idata, buffer);
ret = ioctl(fd, MMC_IOC_CMD, &idata);
// Read CID
idata.write_flag = 0;
idata.opcode = 9;
idata.arg = (1 << 16);
idata.flags = 7;
idata.blksz = 0;
idata.blocks = 0;
mmc_ioc_cmd_set_data(idata, buffer);
ret = ioctl(fd, MMC_IOC_CMD, &idata);
printf("response[0] = %d\n", idata.response[0]);
printf("ret = %d\n", ret);
if(ret)
perror("ioctl");
// Set transition
idata.opcode = 7;
idata.arg = (1 << 16);
idata.flags = MMC_RSP_R1;
idata.blksz = 0;
idata.blocks = 0;
mmc_ioc_cmd_set_data(idata, buffer);
ret = ioctl(fd, MMC_IOC_CMD, &idata);
write(fw, buffer, 16);
free(buffer);
close(fd);
close(fw);
return ret;
}
^ permalink raw reply
* Re: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port
From: Wolfram Sang @ 2016-10-21 21:56 UTC (permalink / raw)
To: Chris Brandt
Cc: Ulf Hansson, Wolfram Sang, Sergei Shtylyov, Geert Uytterhoeven,
Simon Horman, linux-mmc@vger.kernel.org,
linux-renesas-soc@vger.kernel.org
In-Reply-To: <SG2PR06MB1165F5878C8417068E7364628AD40@SG2PR06MB1165.apcprd06.prod.outlook.com>
[-- Attachment #1: Type: text/plain, Size: 556 bytes --]
> For the VERSION register, the low byte is the version of the IP, but
> the upper byte is a number that the design group that made the part
I know. It is just that I haven't seen this one "in the wild" so far.
> > This SWAP register exists on R-Car as well. Out of curiosity, what is the
> > register value of 0xE804E0E4?
>
> 0xE804E0E4 = 0x0001
>
> So...something is there!
I am quite convinced there is this BUSWIDTH register. If you are
interested, try setting this to 0 and see if it works without the 32 bit
register patches.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [RFC v2 1/2] mmc: sdhci: Add device tree property sdhci-cap-speed-modes-broken
From: Zach Brown @ 2016-10-21 21:35 UTC (permalink / raw)
To: ulf.hansson
Cc: adrian.hunter, robh+dt, mark.rutland, linux-mmc, devicetree,
linux-kernel, zach.brown
In-Reply-To: <1477085715-30213-1-git-send-email-zach.brown@ni.com>
On some systems the sdhci capabilty registers are incorrect for one
reason or another.
The sdhci-cap-speed-modes-broken property will let the driver know that
the sdhci capability registers should not be relied on for speed modes.
Instead the driver should check the mmc generic DT bindings.
Signed-off-by: Zach Brown <zach.brown@ni.com>
---
Documentation/devicetree/bindings/mmc/mmc.txt | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
index 8a37782..671d6c0 100644
--- a/Documentation/devicetree/bindings/mmc/mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc.txt
@@ -52,6 +52,9 @@ Optional properties:
- no-sdio: controller is limited to send sdio cmd during initialization
- no-sd: controller is limited to send sd cmd during initialization
- no-mmc: controller is limited to send mmc cmd during initialization
+- sdhci-cap-speed-modes-broken: One or more of the bits in the sdhci
+ capabilities registers representing speed modes are incorrect. All the bits
+ representing speed modes should be ignored.
*NOTE* on CD and WP polarity. To use common for all SD/MMC host controllers line
polarity properties, we have to fix the meaning of the "normal" and "inverted"
--
2.7.4
^ permalink raw reply related
* [RFC v2 2/2] mmc: sdhci: Ignore capability register when it comes to speeds and use DT binding instead when sdhci-cap-speed-modes-broken is set.
From: Zach Brown @ 2016-10-21 21:35 UTC (permalink / raw)
To: ulf.hansson
Cc: adrian.hunter, robh+dt, mark.rutland, linux-mmc, devicetree,
linux-kernel, zach.brown
In-Reply-To: <1477085715-30213-1-git-send-email-zach.brown@ni.com>
When the sdhci-cap-speed-modes-broken DT property is set, the driver
will ignore the bits of the capability registers that correspond to
speed modes.
Signed-off-by: Zach Brown <zach.brown@ni.com>
---
drivers/mmc/host/sdhci.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 1e25b01..59c62d3 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -22,6 +22,7 @@
#include <linux/scatterlist.h>
#include <linux/regulator/consumer.h>
#include <linux/pm_runtime.h>
+#include <linux/of.h>
#include <linux/leds.h>
@@ -3013,10 +3014,19 @@ void __sdhci_read_caps(struct sdhci_host *host, u16 *ver, u32 *caps, u32 *caps1)
host->caps = caps ? *caps : sdhci_readl(host, SDHCI_CAPABILITIES);
+ if (of_property_read_bool(mmc_dev(host->mmc)->of_node,
+ "sdhci-cap-speed-modes-broken"))
+ host->caps &= ~SDHCI_CAN_DO_HISPD;
+
if (host->version < SDHCI_SPEC_300)
return;
host->caps1 = caps1 ? *caps1 : sdhci_readl(host, SDHCI_CAPABILITIES_1);
+
+ if (of_property_read_bool(mmc_dev(host->mmc)->of_node,
+ "sdhci-cap-speed-modes-broken"))
+ host->caps1 &= ~(SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_SDR104 |
+ SDHCI_SUPPORT_DDR50 | SDHCI_SUPPORT_HS400);
}
EXPORT_SYMBOL_GPL(__sdhci_read_caps);
--
2.7.4
^ permalink raw reply related
* [RFC v2 0/2] Ignore capability registers when it comes to speeds and use DT binding instead.
From: Zach Brown @ 2016-10-21 21:35 UTC (permalink / raw)
To: ulf.hansson
Cc: adrian.hunter, robh+dt, mark.rutland, linux-mmc, devicetree,
linux-kernel, zach.brown
The first patch add documentation about a new devicetree property
sdhci-cap-speed-modes-broken.
The second patch makes the sdhci use the DT binding instead of the caps
register for determining which speed modes are supported by the controller.
This RFC is an alternative to another patch[1] set I sent up.
[1]
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1251944.html
v2:
* Removed separate OF parsing function, relying on mmc_of_parse to be called
before read_caps
* Moved check of sdhci-cap-speed-modes-broken into sdhci_read_caps.
* Added SDHCI_SUPPORT_HS400 to list of bits to clear in sdhci cap1, not sure
that's all of the them now, but I think so.
Zach Brown (2):
mmc: sdhci: Add device tree property sdhci-cap-speed-modes-broken
mmc: sdhci: Ignore capability register when it comes to speeds and use
DT binding instead when sdhci-cap-speed-modes-broken is set.
Documentation/devicetree/bindings/mmc/mmc.txt | 3 +++
drivers/mmc/host/sdhci.c | 10 ++++++++++
2 files changed, 13 insertions(+)
--
2.7.4
^ permalink raw reply
* RE: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port
From: Chris Brandt @ 2016-10-21 13:43 UTC (permalink / raw)
To: Wolfram Sang
Cc: Ulf Hansson, Wolfram Sang, Sergei Shtylyov, Geert Uytterhoeven,
Simon Horman, linux-mmc@vger.kernel.org,
linux-renesas-soc@vger.kernel.org
In-Reply-To: <20161020231245.GB2199@katana>
> > > * Does it have a version register (CTL_VERSION)? If so, what does it
> > > say?
> >
> > 0x820B
>
> Okay, this is a version I have not seen before.
The SDHI IP came from the SH7269 (SH-2A device).
For the VERSION register, the low byte is the version of the IP, but the upper byte is a number that the design group that made the part gives it any time they make a change. So, since this IP was from a SH2A team, the numbering might look different than what the SH4A or R-Car teams have used in the past.
> This SWAP register exists on R-Car as well. Out of curiosity, what is the
> register value of 0xE804E0E4?
0xE804E0E4 = 0x0001
So...something is there!
Chris
^ permalink raw reply
* [PATCH] mmc: dw_mmc-pltfm: fix the potential NULL pointer dereference
From: Jaehoon Chung @ 2016-10-21 10:57 UTC (permalink / raw)
To: linux-mmc; +Cc: ulf.hansson, shawn.lin, stable, gregkh, Jaehoon Chung
platform_get_resource can be returned the NULL pointer.
Then regs->start should be referred to NULL Pointer.
devm_ioremap_resource() checks whether res is NULL or not.
Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
---
drivers/mmc/host/dw_mmc-pltfm.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc-pltfm.c b/drivers/mmc/host/dw_mmc-pltfm.c
index b486fef..1236d49b 100644
--- a/drivers/mmc/host/dw_mmc-pltfm.c
+++ b/drivers/mmc/host/dw_mmc-pltfm.c
@@ -47,12 +47,13 @@ int dw_mci_pltfm_register(struct platform_device *pdev,
host->pdata = pdev->dev.platform_data;
regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- /* Get registers' physical base address */
- host->phy_regs = regs->start;
host->regs = devm_ioremap_resource(&pdev->dev, regs);
if (IS_ERR(host->regs))
return PTR_ERR(host->regs);
+ /* Get registers' physical base address */
+ host->phy_regs = regs->start;
+
platform_set_drvdata(pdev, host);
return dw_mci_probe(host);
}
--
2.10.1
^ permalink raw reply related
* Re: [PATCH 4/4] mmc: core: Don't use ->card_busy() and CMD13 in combination when polling
From: Adrian Hunter @ 2016-10-21 9:19 UTC (permalink / raw)
To: Ulf Hansson, linux-mmc; +Cc: Jaehoon Chung, Linus Walleij, Chaotian Jing
In-Reply-To: <1476951579-26125-5-git-send-email-ulf.hansson@linaro.org>
On 20/10/16 11:19, Ulf Hansson wrote:
> When polling for busy after sending a MMC_SWITCH command, both the optional
> ->card_busy() callback and CMD13 are being used in conjunction.
>
> This doesn't make sense. Instead it's more reasonable to rely solely on the
> ->card_busy() callback when it exists. Let's change that and instead use
> the CMD13 as a fall-back. In this way we avoid sending CMD13, unless it's
> really needed.
>
> Within this context, let's also take the opportunity to make some
> additional clean-ups and clarifications to the related code.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> drivers/mmc/core/mmc_ops.c | 30 ++++++++++++++----------------
> 1 file changed, 14 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index a84a880..481bbdb 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -495,34 +495,32 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
> timeout = jiffies + msecs_to_jiffies(timeout_ms) + 1;
> do {
> /*
> - * Due to the possibility of being preempted after
> - * sending the status command, check the expiration
> - * time first.
> + * Due to the possibility of being preempted while polling,
> + * check the expiration time first.
> */
> expired = time_after(jiffies, timeout);
> - if (send_status) {
> +
> + if (host->ops->card_busy) {
> + busy = host->ops->card_busy(host);
I didn't really have time to look at these patches, sorry :-(. But this
loop looks like it could use a cond_resched()
> + } else {
> err = __mmc_send_status(card, &status, ignore_crc);
> if (err)
> return err;
> - }
> - if (host->ops->card_busy) {
> - if (!host->ops->card_busy(host))
> - break;
> - busy = true;
> + busy = R1_CURRENT_STATE(status) == R1_STATE_PRG;
> }
>
> - /* Timeout if the device never leaves the program state. */
> - if (expired &&
> - (R1_CURRENT_STATE(status) == R1_STATE_PRG || busy)) {
> - pr_err("%s: Card stuck in programming state! %s\n",
> + /* Timeout if the device still remains busy. */
> + if (expired && busy) {
> + pr_err("%s: Card stuck being busy! %s\n",
> mmc_hostname(host), __func__);
> return -ETIMEDOUT;
> }
> - } while (R1_CURRENT_STATE(status) == R1_STATE_PRG || busy);
> + } while (busy);
>
> - err = mmc_switch_status_error(host, status);
> + if (host->ops->card_busy && send_status)
> + return mmc_switch_status(card);
>
> - return err;
> + return mmc_switch_status_error(host, status);
> }
>
> /**
>
^ permalink raw reply
* Re: [PATCH] mmc: sdhci-esdhc-imx: make sure usdhc clock enabled while doing suspend
From: Dong Aisheng @ 2016-10-22 1:07 UTC (permalink / raw)
To: Bough Chen; +Cc: Ulf Hansson, Adrian Hunter, linux-mmc, A.S. Dong
In-Reply-To: <AM4PR0401MB23247C544FE16359ACE6847C90D50@AM4PR0401MB2324.eurprd04.prod.outlook.com>
On Thu, Oct 20, 2016 at 10:21:46AM +0000, Bough Chen wrote:
>
>
> > -----Original Message-----
> > From: Dong Aisheng [mailto:dongas86@gmail.com]
> > Sent: Wednesday, October 19, 2016 5:18 PM
> > To: Ulf Hansson <ulf.hansson@linaro.org>
> > Cc: Bough Chen <haibo.chen@nxp.com>; Adrian Hunter
> > <adrian.hunter@intel.com>; linux-mmc <linux-mmc@vger.kernel.org>; A.S.
> > Dong <aisheng.dong@nxp.com>
> > Subject: Re: [PATCH] mmc: sdhci-esdhc-imx: make sure usdhc clock enabled
> > while doing suspend
> >
> > Hi Ulf,
> >
> > On Tue, Oct 18, 2016 at 5:18 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > On 18 October 2016 at 09:39, Haibo Chen <haibo.chen@nxp.com> wrote:
> > >> When suspend usdhc, it will access usdhc register. So usdhc clock
> > >> should be enabled, otherwise the access usdhc register will return
> > >> error or cause system hung.
> > >>
> > >> Take this into consideration, if system enable a usdhc and do not
> > >> connect any SD/SDIO/MMC card, after system boot up, this usdhc will
> > >> do runtime suspend, and close all usdhc clock. At this time, if
> > >> suspend the system, due to no card persent, usdhc runtime resume will
> > >> not be called. So usdhc clock still closed, then in suspend, once
> > >> access usdhc register, system hung or bus error return.
> > >>
> > >> This patch make sure usdhc clock always enabled while doing usdhc
> > >> suspend.
> > >
> > > Yes, and since the clocks are kept enabled during system suspend that
> > > means wasting power, doesn't it!?
> > >
> >
> > IMX SoCs will disable all modules clocks in system stop mode automatically by
> > hardware even it's enabled before CCGR value Clock Activity Description:
> > 00 clock is off during all modes. stop enter hardware handshake is disabled.
> > 01 clock is on in run mode, but off in wait and stop modes
> > 10 Not applicable (Reserved).
> > 11 clock is on during all modes, except stop mode.
> >
> > Although HW will gate off it automatically, but i think it's still good to align the
> > state between SW and HW.
> >
>
> Agree
>
> > > May I propose another solution. Currently you deal only with clock
> > > gating/ungating during runtime suspend/resume. I am wondering whether
> > > you could extend those operations to be similar to what is needed
> > > during system suspend/resume?
> > >
> >
> > IMX driver are calling sdhci_runtime_suspend_host() and
> > sdhci_suspend_host() for runtime
> > suspend and system sleep case respectively.
> > Those two APIs definitions are different.
> > e.g. sdhci_suspend_host will disable card detection and enable wakeup if any
> > while
> > sdhci_runtime_suspend_host() not.
> >
> > It may not be suitable to extend runtime operations to be similar as sleep pm
> > operations if using common sdhci suspend function, unless we implement
> > totoally IMX specific PM/Runtime PM function.
> >
> >
> > Another option may be like what omap_hsmmc does:
> >
> > Something like:
> >
> > int sdhci_esdhc_suspend(struct device *dev) {
> > pm_runtime_get_sync(host->dev);
> > ret = sdhci_pltfm_suspend(dev);
> > pm_runtime_put_sync(host->dev);
> >
> > return ret;
> > }
> >
>
> I think this method still has the same issue.
> According to the /Documentation/power/runtime_pm.txt
>
> 370 int pm_runtime_get_sync(struct device *dev);
> 371 - increment the device's usage counter, run pm_runtime_resume(dev) and
> 372 return its result
>
> 385 int pm_runtime_put_sync(struct device *dev);
> 386 - decrement the device's usage counter; if the result is 0 then run
> 387 pm_runtime_idle(dev) and return its result
>
> pm_runtime_put_sync() will not call pm_runtime_suspend(), so clock still enabled, the same issue.
>
Yes, you're right.
Device may be already in runtime resumed state before.
Regards
Dong Aisheng
> > int sdhci_esdhc_resume(struct device *dev) {
> > pm_runtime_get_sync(host->dev);
> >
> > ...
> > ret = sdhci_pltfm_resume(dev);
> >
> > pm_runtime_mark_last_busy(host->dev);
> > pm_runtime_put_autosuspend(host->dev);
> >
> > return ret;
> > }
> >
> > Does that seem ok?
> >
> > > If that is possible, you can instead deploy the runtime PM centric
> > > approach and get system suspend/resume for free. All you would have to
> > > do is to assign the system PM callbacks to
> > > pm_runtime_force_suspend|resume(). In that way, the above problem
> > > would be solved and you don't need to keep the clocks enabled during
> > > system suspend/resume.
> > >
> > > Kind regards
> > > Uffe
> > >
> >
> > Regards
> > Dong Aisheng
> >
> > >>
> > >> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > >> ---
> > >> drivers/mmc/host/sdhci-esdhc-imx.c | 13 ++++++++++++-
> > >> 1 file changed, 12 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
> > >> b/drivers/mmc/host/sdhci-esdhc-imx.c
> > >> index 7123ef9..1df3846 100644
> > >> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > >> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > >> @@ -1322,17 +1322,28 @@ static int sdhci_esdhc_suspend(struct device
> > >> *dev) {
> > >> struct sdhci_host *host = dev_get_drvdata(dev);
> > >>
> > >> +#ifdef CONFIG_PM
> > >> + pm_runtime_get_sync(host->mmc->parent);
> > >> +#endif
> > >> +
> > >> return sdhci_suspend_host(host); }
> > >>
> > >> static int sdhci_esdhc_resume(struct device *dev) {
> > >> struct sdhci_host *host = dev_get_drvdata(dev);
> > >> + int ret;
> > >>
> > >> /* re-initialize hw state in case it's lost in low power mode */
> > >> sdhci_esdhc_imx_hwinit(host);
> > >> + ret = sdhci_resume_host(host);
> > >>
> > >> - return sdhci_resume_host(host);
> > >> +#ifdef CONFIG_PM
> > >> + pm_runtime_mark_last_busy(host->mmc->parent);
> > >> + pm_runtime_put_autosuspend(host->mmc->parent);
> > >> +#endif
> > >> +
> > >> + return ret;
> > >> }
> > >> #endif
> > >>
> > >> --
> > >> 1.9.1
> > >>
^ permalink raw reply
* Re: [PATCH] mmc: sdhci-esdhc-imx: make sure usdhc clock enabled while doing suspend
From: Dong Aisheng @ 2016-10-22 1:06 UTC (permalink / raw)
To: Ulf Hansson; +Cc: Haibo Chen, Adrian Hunter, linux-mmc, Aisheng Dong
In-Reply-To: <CAPDyKFqig1Ta1yBBF33VOfosR384wwnBWGzJt8VoobUgbBSR8g@mail.gmail.com>
On Fri, Oct 21, 2016 at 09:42:27AM +0200, Ulf Hansson wrote:
> On 19 October 2016 at 11:18, Dong Aisheng <dongas86@gmail.com> wrote:
> > Hi Ulf,
> >
> > On Tue, Oct 18, 2016 at 5:18 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >> On 18 October 2016 at 09:39, Haibo Chen <haibo.chen@nxp.com> wrote:
> >>> When suspend usdhc, it will access usdhc register. So usdhc clock
> >>> should be enabled, otherwise the access usdhc register will return
> >>> error or cause system hung.
> >>>
> >>> Take this into consideration, if system enable a usdhc and do not
> >>> connect any SD/SDIO/MMC card, after system boot up, this usdhc
> >>> will do runtime suspend, and close all usdhc clock. At this time,
> >>> if suspend the system, due to no card persent, usdhc runtime resume
> >>> will not be called. So usdhc clock still closed, then in suspend,
> >>> once access usdhc register, system hung or bus error return.
> >>>
> >>> This patch make sure usdhc clock always enabled while doing usdhc
> >>> suspend.
> >>
> >> Yes, and since the clocks are kept enabled during system suspend that
> >> means wasting power, doesn't it!?
> >>
> >
> > IMX SoCs will disable all modules clocks in system stop mode
> > automatically by hardware
> > even it's enabled before
> > CCGR value Clock Activity Description:
> > 00 clock is off during all modes. stop enter hardware handshake is disabled.
> > 01 clock is on in run mode, but off in wait and stop modes
> > 10 Not applicable (Reserved).
> > 11 clock is on during all modes, except stop mode.
> >
> > Although HW will gate off it automatically, but i think it's still
> > good to align the state between
> > SW and HW.
>
> Yes, indeed!
>
> >
> >> May I propose another solution. Currently you deal only with clock
> >> gating/ungating during runtime suspend/resume. I am wondering whether
> >> you could extend those operations to be similar to what is needed
> >> during system suspend/resume?
> >>
> >
> > IMX driver are calling sdhci_runtime_suspend_host() and
> > sdhci_suspend_host() for runtime
> > suspend and system sleep case respectively.
> > Those two APIs definitions are different.
> > e.g. sdhci_suspend_host will disable card detection and enable wakeup
> > if any while
> > sdhci_runtime_suspend_host() not.
>
> Yes, and that is weird and most likely wrong!
>
> >
> > It may not be suitable to extend runtime operations to be similar as
> > sleep pm operations
>
> I have several times by know had kind of similar discussions, but for
> different sdhci variants. I believe we are papering over a PM issue in
> sdhci, instead of actually trying to solve the real problem and in a
> generic fashion.
>
> As a first step, why not try to combine sdhci_suspend_host() and
> sdhci_runtime_suspend_host() into one function, and vice verse for the
> resume functions.
>
Yes, that's a reasonable way.
We may need double check whether we can combine them into one function
since they're defined for different purpose currently.
> Then sdhci variants can decide how they want to use them.
> Particularly, those that uses runtime PM, should benefit from using
> the runtime PM centric approach and thus get system PM suspend/resume
> for "free".
>
> > if using common sdhci suspend function, unless we implement totoally
> > IMX specific
> > PM/Runtime PM function.
> >
> >
> > Another option may be like what omap_hsmmc does:
> >
> > Something like:
> >
> > int sdhci_esdhc_suspend(struct device *dev)
> > {
> > pm_runtime_get_sync(host->dev);
> > ret = sdhci_pltfm_suspend(dev);
> > pm_runtime_put_sync(host->dev);
> >
> > return ret;
> > }
> >
> > int sdhci_esdhc_resume(struct device *dev)
> > {
> > pm_runtime_get_sync(host->dev);
> >
> > ...
> > ret = sdhci_pltfm_resume(dev);
> >
> > pm_runtime_mark_last_busy(host->dev);
> > pm_runtime_put_autosuspend(host->dev);
> >
> > return ret;
> > }
> >
> > Does that seem ok?
>
> No, this doesn't work!
>
> People that aren't into runtime PM, believes that the
> pm_runtime_put*() in these paths means that the device enters runtime
> suspend state. That's not the case.
>
> Instead the device will remain runtime resumed while the system enters
> suspend. Is that really what you want?
Yes, it's true.
Thanks for spotting it.
>
> [...]
>
> Kind regards
> Uffe
Regards
Dong Aisheng
^ permalink raw reply
* Re: [PATCH 0/4] mmc: core: Clean-up and improve polling code in __mmc_switch()
From: Ulf Hansson @ 2016-10-21 8:22 UTC (permalink / raw)
To: Jaehoon Chung; +Cc: linux-mmc, Adrian Hunter, Linus Walleij, Chaotian Jing
In-Reply-To: <1ba11108-0804-950d-5410-646b53c798e8@samsung.com>
On 21 October 2016 at 09:49, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> On 10/20/2016 05:19 PM, Ulf Hansson wrote:
>> The code in __mmc_switch() is rather messy and hard to follow/understand. This
>> series starts out by cleaning-up and re-factoring that code. In the final patch
>> I intend to improve the behaviour regarding the polling method, which is being
>> used to know when the card stops signal busy.
>
> Looks good to me.
>
> Acked-by: Jaehoon Chung <jh80.chung@samsung.com>
> Tested-by: Jaehoon Chung <jh80.chung@samsung.com>
>
> (Tested on Exynos Series)
>
> Best Regards,
> Jaehoon Chung
Thanks Jaehoon, I have queued them up for my next branch!
Kind regards
Uffe
>
>>
>> Ulf Hansson (4):
>> mmc: core: Make mmc_switch_status() available for mmc core
>> mmc: core: Clarify code which deals with polling in __mmc_switch()
>> mmc: core: Factor out code related to polling in __mmc_switch()
>> mmc: core: Don't use ->card_busy() and CMD13 in combination when
>> polling
>>
>> drivers/mmc/core/mmc.c | 13 -----
>> drivers/mmc/core/mmc_ops.c | 138 +++++++++++++++++++++++++--------------------
>> drivers/mmc/core/mmc_ops.h | 2 +-
>> 3 files changed, 79 insertions(+), 74 deletions(-)
>>
>
^ permalink raw reply
* Re: [PATCH 1/2] mmc: mediatek: Fix module autoload
From: Ulf Hansson @ 2016-10-21 8:22 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: linux-kernel@vger.kernel.org, linux-mmc, Geert Uytterhoeven,
Douglas Anderson, linux-mediatek,
linux-arm-kernel@lists.infradead.org, Chaotian Jing,
Nicolas Boichat, Matthias Brugger
In-Reply-To: <1476720825-30442-1-git-send-email-javier@osg.samsung.com>
On 17 October 2016 at 18:13, Javier Martinez Canillas
<javier@osg.samsung.com> wrote:
> If the driver is built as a module, autoload won't work because the module
> alias information is not filled. So user-space can't match the registered
> device with the corresponding module.
>
> Export the module alias information using the MODULE_DEVICE_TABLE() macro.
>
> Before this patch:
>
> $ modinfo drivers/mmc/host/mtk-sd.ko | grep alias
> $
>
> After this patch:
>
> $ modinfo drivers/mmc/host/mtk-sd.ko | grep alias
> alias: of:N*T*Cmediatek,mt8135-mmcC*
> alias: of:N*T*Cmediatek,mt8135-mmc
>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
Thanks, applied for next!
Kind regards
Uffe
> ---
>
> drivers/mmc/host/mtk-sd.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index 84e9afcb5c09..86af0b199a54 100644
> --- a/drivers/mmc/host/mtk-sd.c
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -1713,6 +1713,7 @@ static const struct of_device_id msdc_of_ids[] = {
> { .compatible = "mediatek,mt8135-mmc", },
> {}
> };
> +MODULE_DEVICE_TABLE(of, msdc_of_ids);
>
> static struct platform_driver mt_msdc_driver = {
> .probe = msdc_drv_probe,
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH v5] mmc: sdhci-msm: Add pm_runtime and system PM support
From: Ulf Hansson @ 2016-10-21 8:22 UTC (permalink / raw)
To: Pramod Gurav
Cc: Adrian Hunter, linux-mmc, linux-kernel@vger.kernel.org,
linux-arm-msm@vger.kernel.org, linux-pm@vger.kernel.org,
Harjani Ritesh
In-Reply-To: <20161021064204.10889-1-pramod.gurav@linaro.org>
On 21 October 2016 at 08:42, Pramod Gurav <pramod.gurav@linaro.org> wrote:
> Provides runtime PM callbacks to enable and disable clock resources
> when idle. Also support system PM callbacks to be called during system
> suspend and resume.
>
> Reviewed-by: Ritesh Harjani <riteshh@codeaurora.org>
> Reviewed-by: Georgi Djakov <georgi.djakov@linaro.org>
> Tested-by: Ritesh Harjani <riteshh@codeaurora.org>
> Signed-off-by: Pramod Gurav <pramod.gurav@linaro.org>
Thanks, applied for next!
Kind regards
Uffe
> ---
> Tested on DB410C.
>
> Changes in v5:
> - Added pm_runtime_mark_last_busy in probe before calling autosuspend
> - included clock names in error logs
> - Used micro instead of constant for autosuspend delay
> - Removed platform_set_drvdata in probe as sdhci_pltfm_init does it.
> - Aligned PM ops structure with the parenthesis
>
> Changes in v4:
> - Remove calls to sdhci_runtime_resume_host/sdhci_runtime_suspend_host
> from runtime callbacks as sdhc msm controller is capable of restoring
> it's register values after clocks are disabled and re-enabled.
>
> Changes in v3:
> - Added CONFIG_PM around runtime pm function.
> - Replaced msm suspend/resume with generic function directly
> - Use SET_SYSTEM_SLEEP_PM_OPS instead of late version
>
> Changes in v2:
> - Moved pm_rutime enabling before adding host
> - Handled pm_rutime in remove
> - Changed runtime handling with reference from sdhci-of-at91.c
>
> drivers/mmc/host/sdhci-msm.c | 68 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 67 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 8ef44a2a..795f16f 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -18,6 +18,7 @@
> #include <linux/of_device.h>
> #include <linux/delay.h>
> #include <linux/mmc/mmc.h>
> +#include <linux/pm_runtime.h>
> #include <linux/slab.h>
>
> #include "sdhci-pltfm.h"
> @@ -68,6 +69,7 @@
> #define CMUX_SHIFT_PHASE_SHIFT 24
> #define CMUX_SHIFT_PHASE_MASK (7 << CMUX_SHIFT_PHASE_SHIFT)
>
> +#define MSM_MMC_AUTOSUSPEND_DELAY_MS 50
> struct sdhci_msm_host {
> struct platform_device *pdev;
> void __iomem *core_mem; /* MSM SDCC mapped address */
> @@ -658,12 +660,26 @@ static int sdhci_msm_probe(struct platform_device *pdev)
> goto clk_disable;
> }
>
> + pm_runtime_get_noresume(&pdev->dev);
> + pm_runtime_set_active(&pdev->dev);
> + pm_runtime_enable(&pdev->dev);
> + pm_runtime_set_autosuspend_delay(&pdev->dev,
> + MSM_MMC_AUTOSUSPEND_DELAY_MS);
> + pm_runtime_use_autosuspend(&pdev->dev);
> +
> ret = sdhci_add_host(host);
> if (ret)
> - goto clk_disable;
> + goto pm_runtime_disable;
> +
> + pm_runtime_mark_last_busy(&pdev->dev);
> + pm_runtime_put_autosuspend(&pdev->dev);
>
> return 0;
>
> +pm_runtime_disable:
> + pm_runtime_disable(&pdev->dev);
> + pm_runtime_set_suspended(&pdev->dev);
> + pm_runtime_put_noidle(&pdev->dev);
> clk_disable:
> clk_disable_unprepare(msm_host->clk);
> pclk_disable:
> @@ -685,6 +701,11 @@ static int sdhci_msm_remove(struct platform_device *pdev)
> 0xffffffff);
>
> sdhci_remove_host(host, dead);
> +
> + pm_runtime_get_sync(&pdev->dev);
> + pm_runtime_disable(&pdev->dev);
> + pm_runtime_put_noidle(&pdev->dev);
> +
> clk_disable_unprepare(msm_host->clk);
> clk_disable_unprepare(msm_host->pclk);
> if (!IS_ERR(msm_host->bus_clk))
> @@ -693,12 +714,57 @@ static int sdhci_msm_remove(struct platform_device *pdev)
> return 0;
> }
>
> +#ifdef CONFIG_PM
> +static int sdhci_msm_runtime_suspend(struct device *dev)
> +{
> + struct sdhci_host *host = dev_get_drvdata(dev);
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +
> + clk_disable_unprepare(msm_host->clk);
> + clk_disable_unprepare(msm_host->pclk);
> +
> + return 0;
> +}
> +
> +static int sdhci_msm_runtime_resume(struct device *dev)
> +{
> + struct sdhci_host *host = dev_get_drvdata(dev);
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> + int ret;
> +
> + ret = clk_prepare_enable(msm_host->clk);
> + if (ret) {
> + dev_err(dev, "clk_enable failed for core_clk: %d\n", ret);
> + return ret;
> + }
> + ret = clk_prepare_enable(msm_host->pclk);
> + if (ret) {
> + dev_err(dev, "clk_enable failed for iface_clk: %d\n", ret);
> + clk_disable_unprepare(msm_host->clk);
> + return ret;
> + }
> +
> + return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops sdhci_msm_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> + pm_runtime_force_resume)
> + SET_RUNTIME_PM_OPS(sdhci_msm_runtime_suspend,
> + sdhci_msm_runtime_resume,
> + NULL)
> +};
> +
> static struct platform_driver sdhci_msm_driver = {
> .probe = sdhci_msm_probe,
> .remove = sdhci_msm_remove,
> .driver = {
> .name = "sdhci_msm",
> .of_match_table = sdhci_msm_dt_match,
> + .pm = &sdhci_msm_pm_ops,
> },
> };
>
> --
> 2.9.3
>
^ permalink raw reply
* Re: [PATCH 2/2] mmc: sdhci-of-at91: Fix module autoload
From: Ulf Hansson @ 2016-10-21 8:21 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: linux-kernel@vger.kernel.org, Ludovic Desroches, linux-mmc,
Adrian Hunter
In-Reply-To: <1476720825-30442-2-git-send-email-javier@osg.samsung.com>
On 17 October 2016 at 18:13, Javier Martinez Canillas
<javier@osg.samsung.com> wrote:
> If the driver is built as a module, autoload won't work because the module
> alias information is not filled. So user-space can't match the registered
> device with the corresponding module.
>
> Export the module alias information using the MODULE_DEVICE_TABLE() macro.
>
> Before this patch:
>
> $ modinfo drivers/mmc/host/sdhci-of-at91.ko | grep alias
> $
>
> After this patch:
>
> $ modinfo drivers/mmc/host/sdhci-of-at91.ko | grep alias
> alias: of:N*T*Catmel,sama5d2-sdhciC*
> alias: of:N*T*Catmel,sama5d2-sdhci
>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
Thanks, applied for next!
Kind regards
Uffe
> ---
>
> drivers/mmc/host/sdhci-of-at91.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> index a9b7fc06c434..2f9ad213377a 100644
> --- a/drivers/mmc/host/sdhci-of-at91.c
> +++ b/drivers/mmc/host/sdhci-of-at91.c
> @@ -100,6 +100,7 @@ static const struct of_device_id sdhci_at91_dt_match[] = {
> { .compatible = "atmel,sama5d2-sdhci", .data = &soc_data_sama5d2 },
> {}
> };
> +MODULE_DEVICE_TABLE(of, sdhci_at91_dt_match);
>
> #ifdef CONFIG_PM
> static int sdhci_at91_runtime_suspend(struct device *dev)
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH] mmc: block: Change MMC_IOC_MAX_BYTES
From: Ulf Hansson @ 2016-10-21 8:21 UTC (permalink / raw)
To: Jeremy Kim; +Cc: linux-mmc, Jaehoon Chung
In-Reply-To: <0d4301d229fc$607a9770$216fc650$@samsung.com>
On 19 October 2016 at 13:31, Jeremy Kim <jh4u.kim@samsung.com> wrote:
> From 27d46f5697434542ad9cafbc6a9630dc14915f91 Mon Sep 17 00:00:00 2001
> From: Jeonghan Kim <jh4u.kim@samsung.com>
> Date: Wed, 19 Oct 2016 19:48:02 +0900
> Subject: [PATCH] mmc: block: Change MMC_IOC_MAX_BYTES
>
> It is used for limitation of buffer size during IOCTL such as FFU.
> However, eMMC FW size is bigger than (512L*256).
> (For instance, currently, Samsung eMMC FW size is over 300KB.)
> So, it needs to increase to execute FFU.
>
> Signed-off-by: Jeonghan Kim <jh4u.kim@samsung.com>
Thanks, applied for next!
Kind regards
Uffe
> ---
> include/uapi/linux/mmc/ioctl.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/mmc/ioctl.h b/include/uapi/linux/mmc/ioctl.h
> index 7e385b8..700a551 100644
> --- a/include/uapi/linux/mmc/ioctl.h
> +++ b/include/uapi/linux/mmc/ioctl.h
> @@ -69,6 +69,6 @@ struct mmc_ioc_multi_cmd {
> * is enforced per ioctl call. For larger data transfers, use the normal
> * block device operations.
> */
> -#define MMC_IOC_MAX_BYTES (512L * 256)
> +#define MMC_IOC_MAX_BYTES (512L * 1024)
> #define MMC_IOC_MAX_CMDS 255
> #endif /* LINUX_MMC_IOCTL_H */
> --
> 1.9.1
>
>
^ permalink raw reply
* Re: [PATCH v2] mmc: sunxi: Prevent against null dereference for vmmc
From: Ulf Hansson @ 2016-10-21 8:21 UTC (permalink / raw)
To: Maxime Ripard
Cc: linux-mmc, linux-kernel@vger.kernel.org, Hans de Goede,
Chen-Yu Tsai, linux-arm-kernel@lists.infradead.org
In-Reply-To: <20161019133304.22915-1-maxime.ripard@free-electrons.com>
On 19 October 2016 at 15:33, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> VMMC is an optional regulator, which means that mmc_regulator_get_supply
> will only return an error in case of a deferred probe, but not when the
> regulator is not set in the DT.
>
> However, the sunxi driver assumes that VMMC is always there, and doesn't
> check the value of the regulator pointer before using it, which obviously
> leads to a (close to) null pointer dereference.
>
> Add proper checks to prevent that.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Thanks, applied for next!
Kind regards
Uffe
>
> ---
> Changes from v1:
> - remove redundant error message
> ---
> drivers/mmc/host/sunxi-mmc.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> index c0a5c676d0e8..b1d1303389a7 100644
> --- a/drivers/mmc/host/sunxi-mmc.c
> +++ b/drivers/mmc/host/sunxi-mmc.c
> @@ -822,10 +822,13 @@ static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> break;
>
> case MMC_POWER_UP:
> - host->ferror = mmc_regulator_set_ocr(mmc, mmc->supply.vmmc,
> - ios->vdd);
> - if (host->ferror)
> - return;
> + if (!IS_ERR(mmc->supply.vmmc)) {
> + host->ferror = mmc_regulator_set_ocr(mmc,
> + mmc->supply.vmmc,
> + ios->vdd);
> + if (host->ferror)
> + return;
> + }
>
> if (!IS_ERR(mmc->supply.vqmmc)) {
> host->ferror = regulator_enable(mmc->supply.vqmmc);
> @@ -847,7 +850,9 @@ static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> case MMC_POWER_OFF:
> dev_dbg(mmc_dev(mmc), "power off!\n");
> sunxi_mmc_reset_host(host);
> - mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0);
> + if (!IS_ERR(mmc->supply.vmmc))
> + mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0);
> +
> if (!IS_ERR(mmc->supply.vqmmc) && host->vqmmc_enabled)
> regulator_disable(mmc->supply.vqmmc);
> host->vqmmc_enabled = false;
> --
> 2.9.3
>
^ permalink raw reply
* Re: [PATCH 0/4] mmc: core: Clean-up and improve polling code in __mmc_switch()
From: Jaehoon Chung @ 2016-10-21 7:49 UTC (permalink / raw)
To: Ulf Hansson, linux-mmc; +Cc: Adrian Hunter, Linus Walleij, Chaotian Jing
In-Reply-To: <1476951579-26125-1-git-send-email-ulf.hansson@linaro.org>
On 10/20/2016 05:19 PM, Ulf Hansson wrote:
> The code in __mmc_switch() is rather messy and hard to follow/understand. This
> series starts out by cleaning-up and re-factoring that code. In the final patch
> I intend to improve the behaviour regarding the polling method, which is being
> used to know when the card stops signal busy.
Looks good to me.
Acked-by: Jaehoon Chung <jh80.chung@samsung.com>
Tested-by: Jaehoon Chung <jh80.chung@samsung.com>
(Tested on Exynos Series)
Best Regards,
Jaehoon Chung
>
> Ulf Hansson (4):
> mmc: core: Make mmc_switch_status() available for mmc core
> mmc: core: Clarify code which deals with polling in __mmc_switch()
> mmc: core: Factor out code related to polling in __mmc_switch()
> mmc: core: Don't use ->card_busy() and CMD13 in combination when
> polling
>
> drivers/mmc/core/mmc.c | 13 -----
> drivers/mmc/core/mmc_ops.c | 138 +++++++++++++++++++++++++--------------------
> drivers/mmc/core/mmc_ops.h | 2 +-
> 3 files changed, 79 insertions(+), 74 deletions(-)
>
^ permalink raw reply
* Re: [PATCH] mmc: sdhci-esdhc-imx: make sure usdhc clock enabled while doing suspend
From: Ulf Hansson @ 2016-10-21 7:42 UTC (permalink / raw)
To: Dong Aisheng; +Cc: Haibo Chen, Adrian Hunter, linux-mmc, Aisheng Dong
In-Reply-To: <CAA+hA=SOk4K6QrQ4T_FCTpuZs1tKfpTA=wHnhvFX7A=30qsLsA@mail.gmail.com>
On 19 October 2016 at 11:18, Dong Aisheng <dongas86@gmail.com> wrote:
> Hi Ulf,
>
> On Tue, Oct 18, 2016 at 5:18 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 18 October 2016 at 09:39, Haibo Chen <haibo.chen@nxp.com> wrote:
>>> When suspend usdhc, it will access usdhc register. So usdhc clock
>>> should be enabled, otherwise the access usdhc register will return
>>> error or cause system hung.
>>>
>>> Take this into consideration, if system enable a usdhc and do not
>>> connect any SD/SDIO/MMC card, after system boot up, this usdhc
>>> will do runtime suspend, and close all usdhc clock. At this time,
>>> if suspend the system, due to no card persent, usdhc runtime resume
>>> will not be called. So usdhc clock still closed, then in suspend,
>>> once access usdhc register, system hung or bus error return.
>>>
>>> This patch make sure usdhc clock always enabled while doing usdhc
>>> suspend.
>>
>> Yes, and since the clocks are kept enabled during system suspend that
>> means wasting power, doesn't it!?
>>
>
> IMX SoCs will disable all modules clocks in system stop mode
> automatically by hardware
> even it's enabled before
> CCGR value Clock Activity Description:
> 00 clock is off during all modes. stop enter hardware handshake is disabled.
> 01 clock is on in run mode, but off in wait and stop modes
> 10 Not applicable (Reserved).
> 11 clock is on during all modes, except stop mode.
>
> Although HW will gate off it automatically, but i think it's still
> good to align the state between
> SW and HW.
Yes, indeed!
>
>> May I propose another solution. Currently you deal only with clock
>> gating/ungating during runtime suspend/resume. I am wondering whether
>> you could extend those operations to be similar to what is needed
>> during system suspend/resume?
>>
>
> IMX driver are calling sdhci_runtime_suspend_host() and
> sdhci_suspend_host() for runtime
> suspend and system sleep case respectively.
> Those two APIs definitions are different.
> e.g. sdhci_suspend_host will disable card detection and enable wakeup
> if any while
> sdhci_runtime_suspend_host() not.
Yes, and that is weird and most likely wrong!
>
> It may not be suitable to extend runtime operations to be similar as
> sleep pm operations
I have several times by know had kind of similar discussions, but for
different sdhci variants. I believe we are papering over a PM issue in
sdhci, instead of actually trying to solve the real problem and in a
generic fashion.
As a first step, why not try to combine sdhci_suspend_host() and
sdhci_runtime_suspend_host() into one function, and vice verse for the
resume functions.
Then sdhci variants can decide how they want to use them.
Particularly, those that uses runtime PM, should benefit from using
the runtime PM centric approach and thus get system PM suspend/resume
for "free".
> if using common sdhci suspend function, unless we implement totoally
> IMX specific
> PM/Runtime PM function.
>
>
> Another option may be like what omap_hsmmc does:
>
> Something like:
>
> int sdhci_esdhc_suspend(struct device *dev)
> {
> pm_runtime_get_sync(host->dev);
> ret = sdhci_pltfm_suspend(dev);
> pm_runtime_put_sync(host->dev);
>
> return ret;
> }
>
> int sdhci_esdhc_resume(struct device *dev)
> {
> pm_runtime_get_sync(host->dev);
>
> ...
> ret = sdhci_pltfm_resume(dev);
>
> pm_runtime_mark_last_busy(host->dev);
> pm_runtime_put_autosuspend(host->dev);
>
> return ret;
> }
>
> Does that seem ok?
No, this doesn't work!
People that aren't into runtime PM, believes that the
pm_runtime_put*() in these paths means that the device enters runtime
suspend state. That's not the case.
Instead the device will remain runtime resumed while the system enters
suspend. Is that really what you want?
[...]
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH 2/2] mmc: sdhci-of-at91: Fix module autoload
From: Adrian Hunter @ 2016-10-21 7:23 UTC (permalink / raw)
To: Javier Martinez Canillas, linux-kernel, linux-mmc, Ulf Hansson
In-Reply-To: <20161021064819.4y5jrdetrqsvibfa@rfolt0960.corp.atmel.com>
On 21/10/16 09:48, Ludovic Desroches wrote:
> On Mon, Oct 17, 2016 at 01:13:45PM -0300, Javier Martinez Canillas wrote:
>> If the driver is built as a module, autoload won't work because the module
>> alias information is not filled. So user-space can't match the registered
>> device with the corresponding module.
>>
>> Export the module alias information using the MODULE_DEVICE_TABLE() macro.
>>
>> Before this patch:
>>
>> $ modinfo drivers/mmc/host/sdhci-of-at91.ko | grep alias
>> $
>>
>> After this patch:
>>
>> $ modinfo drivers/mmc/host/sdhci-of-at91.ko | grep alias
>> alias: of:N*T*Catmel,sama5d2-sdhciC*
>> alias: of:N*T*Catmel,sama5d2-sdhci
>>
>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> Acked-by: Ludovic Desroches <ludovic.desroches@atmel.com>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
>
> Thanks
>> ---
>>
>> drivers/mmc/host/sdhci-of-at91.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
>> index a9b7fc06c434..2f9ad213377a 100644
>> --- a/drivers/mmc/host/sdhci-of-at91.c
>> +++ b/drivers/mmc/host/sdhci-of-at91.c
>> @@ -100,6 +100,7 @@ static const struct of_device_id sdhci_at91_dt_match[] = {
>> { .compatible = "atmel,sama5d2-sdhci", .data = &soc_data_sama5d2 },
>> {}
>> };
>> +MODULE_DEVICE_TABLE(of, sdhci_at91_dt_match);
>>
>> #ifdef CONFIG_PM
>> static int sdhci_at91_runtime_suspend(struct device *dev)
>> --
>> 2.7.4
>>
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox