* [RFC PATCH 7/8] mmc: queue: remove BUG_ON for bounce_sg
From: Shawn Lin @ 2016-10-18 12:04 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Shawn Lin
In-Reply-To: <1476792192-6265-1-git-send-email-shawn.lin@rock-chips.com>
bounce_sg for mqrq_cur and mqrq_pre are proper
allocated when initializing the queue and will not
be freed before explicitly cleaning the queue. So from
the code itself it should be quite confident to remove
this check. If that BUG_ON take effects, it is mostly
likely the memory is randomly oopsing.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/card/queue.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index 8037f73..6c8978a 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -505,8 +505,6 @@ unsigned int mmc_queue_map_sg(struct mmc_queue *mq, struct mmc_queue_req *mqrq)
return blk_rq_map_sg(mq->queue, mqrq->req, mqrq->sg);
}
- BUG_ON(!mqrq->bounce_sg);
-
if (mmc_packed_cmd(cmd_type))
sg_len = mmc_queue_packed_map_sg(mq, mqrq->packed,
mqrq->bounce_sg, cmd_type);
--
2.3.7
^ permalink raw reply related
* [RFC PATCH 6/8] mmc: sdio_uart: remove meaningless BUG_ON
From: Shawn Lin @ 2016-10-18 12:04 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Shawn Lin
In-Reply-To: <1476792192-6265-1-git-send-email-shawn.lin@rock-chips.com>
The code seems quite simple to maintain the sdio_uart_table,
and the insert/remove port from the table are symmetric. If
the BUG_ON occurs, which means serial_core modify the index
or mess up the port sequence anyway.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/card/sdio_uart.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mmc/card/sdio_uart.c b/drivers/mmc/card/sdio_uart.c
index 5af6fb9..491c187 100644
--- a/drivers/mmc/card/sdio_uart.c
+++ b/drivers/mmc/card/sdio_uart.c
@@ -135,8 +135,6 @@ static void sdio_uart_port_remove(struct sdio_uart_port *port)
{
struct sdio_func *func;
- BUG_ON(sdio_uart_table[port->index] != port);
-
spin_lock(&sdio_uart_table_lock);
sdio_uart_table[port->index] = NULL;
spin_unlock(&sdio_uart_table_lock);
--
2.3.7
^ permalink raw reply related
* [RFC PATCH 5/8] mmc: core: remove BUG_ONs from core.c
From: Shawn Lin @ 2016-10-18 12:04 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Shawn Lin
In-Reply-To: <1476792192-6265-1-git-send-email-shawn.lin@rock-chips.com>
BUG_ONs doesn't help anything except for stop the system from
running. If it occurs, it implies we should deploy proper error
handling for that. So this patch is gonna discard these meaningless
BUG_ONs and deploy error handling if needed.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/core/core.c | 40 ++++++++++++++--------------------------
1 file changed, 14 insertions(+), 26 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 2553d90..d8b166b 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -306,16 +306,16 @@ static int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
mrq->sbc->mrq = mrq;
}
if (mrq->data) {
- BUG_ON(mrq->data->blksz > host->max_blk_size);
- BUG_ON(mrq->data->blocks > host->max_blk_count);
- BUG_ON(mrq->data->blocks * mrq->data->blksz >
- host->max_req_size);
-
+ if (mrq->data->blksz > host->max_blk_size ||
+ mrq->data->blocks > host->max_blk_count ||
+ mrq->data->blocks * mrq->data->blksz > host->max_req_size)
+ return -EINVAL;
#ifdef CONFIG_MMC_DEBUG
sz = 0;
for_each_sg(mrq->data->sg, sg, mrq->data->sg_len, i)
sz += sg->length;
- BUG_ON(sz != mrq->data->blocks * mrq->data->blksz);
+ if (sz != mrq->data->blocks * mrq->data->blksz)
+ return -EINVAL;
#endif
mrq->cmd->data = mrq->data;
@@ -349,9 +349,8 @@ void mmc_start_bkops(struct mmc_card *card, bool from_exception)
int timeout;
bool use_busy_signal;
- BUG_ON(!card);
-
- if (!card->ext_csd.man_bkops_en || mmc_card_doing_bkops(card))
+ if (!card || !card->ext_csd.man_bkops_en ||
+ mmc_card_doing_bkops(card))
return;
err = mmc_read_bkops_status(card);
@@ -754,8 +753,6 @@ int mmc_interrupt_hpi(struct mmc_card *card)
u32 status;
unsigned long prg_wait;
- BUG_ON(!card);
-
if (!card->ext_csd.hpi_en) {
pr_info("%s: HPI enable bit unset\n", mmc_hostname(card->host));
return 1;
@@ -850,7 +847,9 @@ int mmc_stop_bkops(struct mmc_card *card)
{
int err = 0;
- BUG_ON(!card);
+ if (!card)
+ return -EINVAL;
+
err = mmc_interrupt_hpi(card);
/*
@@ -1666,8 +1665,6 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage, u32 ocr)
int err = 0;
u32 clock;
- BUG_ON(!host);
-
/*
* Send CMD11 only if the request is to switch the card to
* 1.8V signalling.
@@ -1884,9 +1881,7 @@ void mmc_power_cycle(struct mmc_host *host, u32 ocr)
*/
static void __mmc_release_bus(struct mmc_host *host)
{
- BUG_ON(!host);
- BUG_ON(host->bus_refs);
- BUG_ON(!host->bus_dead);
+ WARN_ON(!host->bus_dead);
host->bus_ops = NULL;
}
@@ -1926,15 +1921,12 @@ void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops *ops)
{
unsigned long flags;
- BUG_ON(!host);
- BUG_ON(!ops);
-
WARN_ON(!host->claimed);
spin_lock_irqsave(&host->lock, flags);
- BUG_ON(host->bus_ops);
- BUG_ON(host->bus_refs);
+ WARN_ON(host->bus_ops);
+ WARN_ON(host->bus_refs);
host->bus_ops = ops;
host->bus_refs = 1;
@@ -1950,8 +1942,6 @@ void mmc_detach_bus(struct mmc_host *host)
{
unsigned long flags;
- BUG_ON(!host);
-
WARN_ON(!host->claimed);
WARN_ON(!host->bus_ops);
@@ -2865,8 +2855,6 @@ void mmc_stop_host(struct mmc_host *host)
}
mmc_bus_put(host);
- BUG_ON(host->card);
-
mmc_claim_host(host);
mmc_power_off(host);
mmc_release_host(host);
--
2.3.7
^ permalink raw reply related
* [RFC PATCH 4/8] mmc: core: remove BUG_ONs from sd
From: Shawn Lin @ 2016-10-18 12:04 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Shawn Lin
In-Reply-To: <1476792192-6265-1-git-send-email-shawn.lin@rock-chips.com>
BUG_ONs doesn't help anything except for stop the system from
running. If it occurs, it implies we should deploy proper error
handling for that. So this patch is gonna discard these meaningless
BUG_ONs and deploy error handling if needed.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/core/sd.c | 20 +++++++++-----------
drivers/mmc/core/sd_ops.c | 30 ++++++++++--------------------
2 files changed, 19 insertions(+), 31 deletions(-)
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 73c762a..bd3ddcc 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -927,7 +927,6 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
u32 cid[4];
u32 rocr = 0;
- BUG_ON(!host);
WARN_ON(!host->claimed);
err = mmc_sd_get_cid(host, ocr, cid, &rocr);
@@ -1043,9 +1042,6 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
*/
static void mmc_sd_remove(struct mmc_host *host)
{
- BUG_ON(!host);
- BUG_ON(!host->card);
-
mmc_remove_card(host->card);
host->card = NULL;
}
@@ -1065,8 +1061,8 @@ static void mmc_sd_detect(struct mmc_host *host)
{
int err;
- BUG_ON(!host);
- BUG_ON(!host->card);
+ if (!host)
+ return;
mmc_get_card(host->card);
@@ -1091,8 +1087,8 @@ static int _mmc_sd_suspend(struct mmc_host *host)
{
int err = 0;
- BUG_ON(!host);
- BUG_ON(!host->card);
+ if (!host)
+ return -EINVAL;
mmc_claim_host(host);
@@ -1136,8 +1132,8 @@ static int _mmc_sd_resume(struct mmc_host *host)
{
int err = 0;
- BUG_ON(!host);
- BUG_ON(!host->card);
+ if (!host)
+ return -EINVAL;
mmc_claim_host(host);
@@ -1221,7 +1217,9 @@ int mmc_attach_sd(struct mmc_host *host)
int err;
u32 ocr, rocr;
- BUG_ON(!host);
+ if (!host)
+ return -EINVAL;
+
WARN_ON(!host->claimed);
err = mmc_send_app_op_cond(host, 0, &ocr);
diff --git a/drivers/mmc/core/sd_ops.c b/drivers/mmc/core/sd_ops.c
index 16b774c..a2cc157 100644
--- a/drivers/mmc/core/sd_ops.c
+++ b/drivers/mmc/core/sd_ops.c
@@ -27,8 +27,8 @@ int mmc_app_cmd(struct mmc_host *host, struct mmc_card *card)
int err;
struct mmc_command cmd = {0};
- BUG_ON(!host);
- BUG_ON(card && (card->host != host));
+ if (WARN_ON(card && card->host != host))
+ return -EINVAL;
cmd.opcode = MMC_APP_CMD;
@@ -72,8 +72,8 @@ int mmc_wait_for_app_cmd(struct mmc_host *host, struct mmc_card *card,
int i, err;
- BUG_ON(!cmd);
- BUG_ON(retries < 0);
+ if (retries < 0)
+ retries = MMC_CMD_RETRIES;
err = -EIO;
@@ -122,9 +122,6 @@ int mmc_app_set_bus_width(struct mmc_card *card, int width)
{
struct mmc_command cmd = {0};
- BUG_ON(!card);
- BUG_ON(!card->host);
-
cmd.opcode = SD_APP_SET_BUS_WIDTH;
cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
@@ -147,8 +144,6 @@ int mmc_send_app_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr)
struct mmc_command cmd = {0};
int i, err = 0;
- BUG_ON(!host);
-
cmd.opcode = SD_APP_OP_COND;
if (mmc_host_is_spi(host))
cmd.arg = ocr & (1 << 30); /* SPI only defines one bit */
@@ -224,8 +219,8 @@ int mmc_send_relative_addr(struct mmc_host *host, unsigned int *rca)
int err;
struct mmc_command cmd = {0};
- BUG_ON(!host);
- BUG_ON(!rca);
+ if (!rca)
+ return -ENOMEM;
cmd.opcode = SD_SEND_RELATIVE_ADDR;
cmd.arg = 0;
@@ -249,9 +244,8 @@ int mmc_app_send_scr(struct mmc_card *card, u32 *scr)
struct scatterlist sg;
void *data_buf;
- BUG_ON(!card);
- BUG_ON(!card->host);
- BUG_ON(!scr);
+ if (!scr)
+ return -ENOMEM;
/* NOTE: caller guarantees scr is heap-allocated */
@@ -307,9 +301,6 @@ int mmc_sd_switch(struct mmc_card *card, int mode, int group,
struct mmc_data data = {0};
struct scatterlist sg;
- BUG_ON(!card);
- BUG_ON(!card->host);
-
/* NOTE: caller guarantees resp is heap-allocated */
mode = !!mode;
@@ -352,9 +343,8 @@ int mmc_app_sd_status(struct mmc_card *card, void *ssr)
struct mmc_data data = {0};
struct scatterlist sg;
- BUG_ON(!card);
- BUG_ON(!card->host);
- BUG_ON(!ssr);
+ if (!ssr)
+ return -ENOMEM;
/* NOTE: caller guarantees ssr is heap-allocated */
--
2.3.7
^ permalink raw reply related
* [RFC PATCH 3/8] mmc: core: remove BUG_ONs from mmc
From: Shawn Lin @ 2016-10-18 12:04 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Shawn Lin
In-Reply-To: <1476792192-6265-1-git-send-email-shawn.lin@rock-chips.com>
BUG_ONs doesn't help anything except for stop the system from
running. If it occurs, it implies we should deploy proper error
handling for that. So this patch is gonna discard these meaningless
BUG_ONs and deploy error handling if needed.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/core/mmc.c | 24 ++++++++++++++----------
drivers/mmc/core/mmc_ops.c | 20 ++++++--------------
2 files changed, 20 insertions(+), 24 deletions(-)
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 39fc5b2..8f63b59 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1477,7 +1477,9 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
u32 cid[4];
u32 rocr;
- BUG_ON(!host);
+ if (!host)
+ return -EINVAL;
+
WARN_ON(!host->claimed);
/* Set correct bus mode for MMC before attempting init */
@@ -1867,8 +1869,8 @@ static int mmc_poweroff_notify(struct mmc_card *card, unsigned int notify_type)
*/
static void mmc_remove(struct mmc_host *host)
{
- BUG_ON(!host);
- BUG_ON(!host->card);
+ if (!host)
+ return;
mmc_remove_card(host->card);
host->card = NULL;
@@ -1889,8 +1891,8 @@ static void mmc_detect(struct mmc_host *host)
{
int err;
- BUG_ON(!host);
- BUG_ON(!host->card);
+ if (WARN_ON(!host))
+ return;
mmc_get_card(host->card);
@@ -1917,8 +1919,8 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT :
EXT_CSD_POWER_OFF_LONG;
- BUG_ON(!host);
- BUG_ON(!host->card);
+ if (WARN_ON(!host))
+ return -EINVAL;
mmc_claim_host(host);
@@ -1976,8 +1978,8 @@ static int _mmc_resume(struct mmc_host *host)
{
int err = 0;
- BUG_ON(!host);
- BUG_ON(!host->card);
+ if (WARN_ON(!host))
+ return -EINVAL;
mmc_claim_host(host);
@@ -2111,7 +2113,9 @@ int mmc_attach_mmc(struct mmc_host *host)
int err;
u32 ocr, rocr;
- BUG_ON(!host);
+ if (!host)
+ return -EINVAL;
+
WARN_ON(!host->claimed);
/* Set correct bus mode for MMC before attempting attach */
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index ad6e979..ceb41eb 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -60,9 +60,6 @@ static inline int __mmc_send_status(struct mmc_card *card, u32 *status,
int err;
struct mmc_command cmd = {0};
- BUG_ON(!card);
- BUG_ON(!card->host);
-
cmd.opcode = MMC_SEND_STATUS;
if (!mmc_host_is_spi(card->host))
cmd.arg = card->rca << 16;
@@ -92,7 +89,8 @@ static int _mmc_select_card(struct mmc_host *host, struct mmc_card *card)
{
struct mmc_command cmd = {0};
- BUG_ON(!host);
+ if (!host)
+ return -EINVAL;
cmd.opcode = MMC_SELECT_CARD;
@@ -109,7 +107,6 @@ static int _mmc_select_card(struct mmc_host *host, struct mmc_card *card)
int mmc_select_card(struct mmc_card *card)
{
- BUG_ON(!card);
return _mmc_select_card(card->host, card);
}
@@ -181,8 +178,6 @@ int mmc_send_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr)
struct mmc_command cmd = {0};
int i, err = 0;
- BUG_ON(!host);
-
cmd.opcode = MMC_SEND_OP_COND;
cmd.arg = mmc_host_is_spi(host) ? 0 : ocr;
cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R3 | MMC_CMD_BCR;
@@ -221,8 +216,8 @@ int mmc_all_send_cid(struct mmc_host *host, u32 *cid)
int err;
struct mmc_command cmd = {0};
- BUG_ON(!host);
- BUG_ON(!cid);
+ if (!cid)
+ return -ENOMEM;
cmd.opcode = MMC_ALL_SEND_CID;
cmd.arg = 0;
@@ -241,9 +236,6 @@ int mmc_set_relative_addr(struct mmc_card *card)
{
struct mmc_command cmd = {0};
- BUG_ON(!card);
- BUG_ON(!card->host);
-
cmd.opcode = MMC_SET_RELATIVE_ADDR;
cmd.arg = card->rca << 16;
cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
@@ -257,8 +249,8 @@ mmc_send_cxd_native(struct mmc_host *host, u32 arg, u32 *cxd, int opcode)
int err;
struct mmc_command cmd = {0};
- BUG_ON(!host);
- BUG_ON(!cxd);
+ if (!cxd)
+ return -ENOMEM;
cmd.opcode = opcode;
cmd.arg = arg;
--
2.3.7
^ permalink raw reply related
* [RFC PATCH 2/8] mmc: debugfs: remove BUG_ON from mmc_ext_csd_open
From: Shawn Lin @ 2016-10-18 12:03 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Shawn Lin
In-Reply-To: <1476792192-6265-1-git-send-email-shawn.lin@rock-chips.com>
Return error value for file_operations callback instead
of triggering BUG_ON which is meaningless. Personally I
don't believe n != EXT_CSD_STR_LEN could happen. Anyway,
propagate the error to the caller.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/core/debugfs.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index c8451ce..30623b8 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -321,7 +321,11 @@ static int mmc_ext_csd_open(struct inode *inode, struct file *filp)
for (i = 0; i < 512; i++)
n += sprintf(buf + n, "%02x", ext_csd[i]);
n += sprintf(buf + n, "\n");
- BUG_ON(n != EXT_CSD_STR_LEN);
+
+ if (n != EXT_CSD_STR_LEN) {
+ err = -EINVAL;
+ goto out_free;
+ }
filp->private_data = buf;
kfree(ext_csd);
--
2.3.7
^ permalink raw reply related
* [RFC PATCH 1/8] mmc: core: remove BUG_ONs from sdio
From: Shawn Lin @ 2016-10-18 12:03 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Shawn Lin
In-Reply-To: <1476792192-6265-1-git-send-email-shawn.lin@rock-chips.com>
BUG_ONs doesn't help anything except for stop the system from
running. If it occurs, it implies we should deploy proper error
handling for that. So this patch is gonna discard these meaningless
BUG_ONs and deploy error handling if needed.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/core/sdio.c | 26 ++++++++++++++------------
drivers/mmc/core/sdio_cis.c | 3 ++-
drivers/mmc/core/sdio_irq.c | 12 +++++++-----
3 files changed, 23 insertions(+), 18 deletions(-)
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index bd44ba8..80883c6 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -63,7 +63,8 @@ static int sdio_init_func(struct mmc_card *card, unsigned int fn)
int ret;
struct sdio_func *func;
- BUG_ON(fn > SDIO_MAX_FUNCS);
+ if (WARN_ON(fn > SDIO_MAX_FUNCS))
+ return -EINVAL;
func = sdio_alloc_func(card);
if (IS_ERR(func))
@@ -555,7 +556,9 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
u32 rocr = 0;
u32 ocr_card = ocr;
- BUG_ON(!host);
+ if (!host)
+ return -EINVAL;
+
WARN_ON(!host->claimed);
/* to query card if 1.8V signalling is supported */
@@ -791,8 +794,8 @@ static void mmc_sdio_remove(struct mmc_host *host)
{
int i;
- BUG_ON(!host);
- BUG_ON(!host->card);
+ if (!host)
+ return;
for (i = 0;i < host->card->sdio_funcs;i++) {
if (host->card->sdio_func[i]) {
@@ -820,8 +823,8 @@ static void mmc_sdio_detect(struct mmc_host *host)
{
int err;
- BUG_ON(!host);
- BUG_ON(!host->card);
+ if (WARN_ON(!host))
+ goto out;
/* Make sure card is powered before detecting it */
if (host->caps & MMC_CAP_POWER_OFF_CARD) {
@@ -916,8 +919,8 @@ static int mmc_sdio_resume(struct mmc_host *host)
{
int err = 0;
- BUG_ON(!host);
- BUG_ON(!host->card);
+ if (!host)
+ return -EINVAL;
/* Basic card reinitialization. */
mmc_claim_host(host);
@@ -970,9 +973,6 @@ static int mmc_sdio_power_restore(struct mmc_host *host)
{
int ret;
- BUG_ON(!host);
- BUG_ON(!host->card);
-
mmc_claim_host(host);
/*
@@ -1063,7 +1063,9 @@ int mmc_attach_sdio(struct mmc_host *host)
u32 ocr, rocr;
struct mmc_card *card;
- BUG_ON(!host);
+ if (!host)
+ return -EINVAL;
+
WARN_ON(!host->claimed);
err = mmc_send_io_op_cond(host, 0, &ocr);
diff --git a/drivers/mmc/core/sdio_cis.c b/drivers/mmc/core/sdio_cis.c
index dcb3dee..f8c3728 100644
--- a/drivers/mmc/core/sdio_cis.c
+++ b/drivers/mmc/core/sdio_cis.c
@@ -262,7 +262,8 @@ static int sdio_read_cis(struct mmc_card *card, struct sdio_func *func)
else
prev = &card->tuples;
- BUG_ON(*prev);
+ if (*prev)
+ return -EINVAL;
do {
unsigned char tpl_code, tpl_link;
diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
index 91bbbfb..f1faf9a 100644
--- a/drivers/mmc/core/sdio_irq.c
+++ b/drivers/mmc/core/sdio_irq.c
@@ -214,7 +214,9 @@ static int sdio_card_irq_put(struct mmc_card *card)
struct mmc_host *host = card->host;
WARN_ON(!host->claimed);
- BUG_ON(host->sdio_irqs < 1);
+
+ if (host->sdio_irqs < 1)
+ return -EINVAL;
if (!--host->sdio_irqs) {
if (!(host->caps2 & MMC_CAP2_SDIO_IRQ_NOTHREAD)) {
@@ -261,8 +263,8 @@ int sdio_claim_irq(struct sdio_func *func, sdio_irq_handler_t *handler)
int ret;
unsigned char reg;
- BUG_ON(!func);
- BUG_ON(!func->card);
+ if (!func)
+ return -EINVAL;
pr_debug("SDIO: Enabling IRQ for %s...\n", sdio_func_id(func));
@@ -304,8 +306,8 @@ int sdio_release_irq(struct sdio_func *func)
int ret;
unsigned char reg;
- BUG_ON(!func);
- BUG_ON(!func->card);
+ if (!func)
+ return -EINVAL;
pr_debug("SDIO: Disabling IRQ for %s...\n", sdio_func_id(func));
--
2.3.7
^ permalink raw reply related
* [RFC PATCH 0/8] Attempt to clean up BUG_ONs for mmc-tree
From: Shawn Lin @ 2016-10-18 12:03 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Shawn Lin
We have already removed some BUG_ONs recently[0][1], however there
are still many pointless BUG_ONs for mmc core. So this patchset is
going on removing these BUG_ONs from drivers/mmc/core/* and drivers/mmc/card/*.
It is also possible for us to remove it from drivers/mmc/host/* in the future.
No functional changes intented in theory, except for adding some error
handle, but I held it for a while in order to test it fully and decided
to send it after the merge window of 4.9.
Please review and comment. :)
[0]: https://patchwork.kernel.org/patch/9300739/
[1]: https://patchwork.kernel.org/patch/9300741/
Shawn Lin (8):
mmc: core: remove BUG_ONs from sdio
mmc: debugfs: remove BUG_ON from mmc_ext_csd_open
mmc: core: remove BUG_ONs from mmc
mmc: core: remove BUG_ONs from sd
mmc: core: remove BUG_ONs from core.c
mmc: sdio_uart: remove meaningless BUG_ON
mmc: queue: remove BUG_ON for bounce_sg
mmc: mmc_test: remove BUG_ONs and deploy error handling
drivers/mmc/card/mmc_test.c | 12 ++++++++----
drivers/mmc/card/queue.c | 2 --
drivers/mmc/card/sdio_uart.c | 2 --
drivers/mmc/core/core.c | 40 ++++++++++++++--------------------------
drivers/mmc/core/debugfs.c | 6 +++++-
drivers/mmc/core/mmc.c | 24 ++++++++++++++----------
drivers/mmc/core/mmc_ops.c | 20 ++++++--------------
drivers/mmc/core/sd.c | 20 +++++++++-----------
drivers/mmc/core/sd_ops.c | 30 ++++++++++--------------------
drivers/mmc/core/sdio.c | 26 ++++++++++++++------------
drivers/mmc/core/sdio_cis.c | 3 ++-
drivers/mmc/core/sdio_irq.c | 12 +++++++-----
12 files changed, 89 insertions(+), 108 deletions(-)
--
2.3.7
^ permalink raw reply
* Re: [PATCH v2 0/9] Init runtime PM support for dw_mmc
From: Shawn Lin @ 2016-10-18 11:35 UTC (permalink / raw)
To: Ulf Hansson
Cc: shawn.lin, Jaehoon Chung, linux-mmc, Doug Anderson,
open list:ARM/Rockchip SoC..., Heiko Stuebner
In-Reply-To: <CAPDyKFopNgWh=+0RNFwJc+aaFPXGuJMAac5rPtfOawvLwSbN+g@mail.gmail.com>
在 2016/10/18 16:46, Ulf Hansson 写道:
> + Heiko
>
> On 12 October 2016 at 04:50, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>
>> Hi Jaehoon and Ulf,
>>
>> This patch is gonna support runtime PM for dw_mmc.
>> It could support to disable ciu_clk by default and disable
>> biu_clk if the devices are non-removeable, or removeable
>> with gpio-base card detect.
>>
>> Then I remove the system PM since the runtime PM actually
>> does the same thing as it. So I help migrate the dw_mmc variant
>> drivers to use runtime PM pairs and pm_runtime_force_*. Note
>> that I only enable runtime PM for dw_mmc-rockchip as I will
>> leave the decision to the owners of the corresponding drivers.
>> I just tested it on my RK3288 platform with linux-next to make
>> the runtime PM and system PM work fine for my emmc, sd card and
>> sdio. But I don't have hardware to help test other variant drivers.
>> But in theory it should work fine as I mentioned that the runtime
>> PM does the same thing as system PM except for disabling ciu_clk
>> aggressively which should not be related to the variant hosts.
>>
>> As you could see that I just extend the slot-gpio a bit, so the
>> ideal way is Ulf could pick them up with Jaehoon's ack. :)
>
> The mmc core change looks fine to me, so I will wait for a pull
> request from Jaehoon.
>
>>
>>
>> Changes in v2:
>> - use struct device as argument for runtime callback
>> - use dw_mci_runtime_* directly
>> - use dw_mci_runtime_* directly
>> - minor fix since I change the argument for dw_mci_runtime_*
>> - use dw_mci_runtime_* directly
>> - use dw_mci_runtime_* directly
>>
>> 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(-)
>>
>
> Overall these changes looks good to me, so I am ready to accept the PR
> from Jaehoon!!
>
>
> Although, highly related to this patchset, I am worried that there is
> a misunderstanding on how MMC_PM_KEEP_POWER (DT binding
> "keep-power-in-suspend") is being used for dw_mmc. Perhaps I am wrong,
> but I would appreciate if you could elaborate a bit for my
> understanding.
>
> First, this cap is solely intended to be used for controllers which
> may have SDIO cards attached, as it indicates those cards may be
> configured to be powered on while the system enters suspend state. By
> looking at some DTS files, for example
> arch/arm64/boot/dts/rockchip/rk3368-orion-r68-meta.dts which uses it
> for an eMMC slot, this is clearly being abused.
Indeed. In general, it should be copy-paste issues as folks maybe write
their dts referring to the exist dts there. So yes, I will do some
cleanup work for them in prevent of further spread of abused code.
>
> Anyway, the wrong DT configurations might not be a big deal, as in
> dw_mci_resume(), it's not the capabilities bit that is checked but the
> corresponding "pm_flag". This flag is set via calling
> sdio_set_host_pm_flags(), but as that doesn't happen for an eMMC card
> we should be fine, right!?
>
> Now, what also do puzzles me, is exactly that piece of code in
> dw_mci_resume() that is being executed *when* the pm_flag contains
> MMC_PM_KEEP_POWER:
> if (slot->mmc->pm_flags & MMC_PM_KEEP_POWER) {
> dw_mci_set_ios(slot->mmc, &slot->mmc->ios);
> dw_mci_setup_bus(slot, true);
> }
>
> So, in the system resume path, the above do makes sense as you need to
> restore the registers etc for the dw_mmc controller to enable it to
> operate the SDIO card. Such as bus width, clocks, etc.
>
> Although, I would expect something similar would be needed in the new
> runtime resume path as well. And in particular also for eMMC/SD cards,
> as you need to restore the dw_mmc registers to be able to operate the
> card again. Don't you?
yes, we do.
>
> So in the end, perhaps you should *always* call dw_mci_set_ios() and
> dw_mci_setup_bus() in dw_mci_resume() instead of conditionally check
> for MMC_PM_KEEP_POWER? Or maybe only a subset of those functions?
>
Thanks for noticing this.
Personally, I realize there is no need to check MMC_PM_KEEP_POWER but
it could be highly related to the cost of S-2-R, I guess. I just checked
sdhci and saw the similar cases you mentioned at the first glance.
Maybe I'm wrong but I need more time to investigate this issue later.
There are still some on-going cleanup work for dw_mmc listed on my TODO
list, including bugfix, legacy/redundant code etc.. So I will check this
one either. Maybe Jaehoon could also do some stree test on enxyos
platforms. :)
> Kind regards
> Uffe
>
>
>
--
Best Regards
Shawn Lin
^ permalink raw reply
* Re: [PATCH v2 0/9] Init runtime PM support for dw_mmc
From: Ulf Hansson @ 2016-10-18 11:15 UTC (permalink / raw)
To: Jaehoon Chung
Cc: open list:ARM/Rockchip SoC..., Shawn Lin, linux-mmc,
Doug Anderson, Heiko Stuebner
In-Reply-To: <17f821c0-46d3-76e7-06dd-fbb415de37eb-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
[...]
>>
>> Now, what also do puzzles me, is exactly that piece of code in
>> dw_mci_resume() that is being executed *when* the pm_flag contains
>> MMC_PM_KEEP_POWER:
>> if (slot->mmc->pm_flags & MMC_PM_KEEP_POWER) {
>> dw_mci_set_ios(slot->mmc, &slot->mmc->ios);
>> dw_mci_setup_bus(slot, true);
>> }
>
> There is no place to set this in dw_mmc controller..Maybe it should be located in other vendor driver.
> Almost all cards that used SDIO might be WiFi..Of course..there might be other cards.
> e.g) In exnyos, use the sdio as WiFi card(broadcom chipset) then broadcom driver also used..
> When broadcom driver initialized or suspending time, it should be set to this flag..
> As i mentioned..it's not normal case.
>
>>
>> So, in the system resume path, the above do makes sense as you need to
>> restore the registers etc for the dw_mmc controller to enable it to
>> operate the SDIO card. Such as bus width, clocks, etc.
>>
>> Although, I would expect something similar would be needed in the new
>> runtime resume path as well. And in particular also for eMMC/SD cards,
>> as you need to restore the dw_mmc registers to be able to operate the
>> card again. Don't you?
>
> Need to check :)
>
>>
>> So in the end, perhaps you should *always* call dw_mci_set_ios() and
>> dw_mci_setup_bus() in dw_mci_resume() instead of conditionally check
>> for MMC_PM_KEEP_POWER? Or maybe only a subset of those functions?
>
> It's reasonable..but it also needs to check on real case and stress test.
Yes, and we can try out that later on top of this series.
Anyway, I wanted to highlight my concern, as I think it's only a
matter of time before we needed this on for runtime PM for some SoCs
that uses dw_mmc.
Kind regards
Uffe
^ permalink raw reply
* RE: [PATCH] mmc: block: Change MMC_IOC_MAX_BYTES
From: Jeremy Kim @ 2016-10-18 10:54 UTC (permalink / raw)
To: 'Jaehoon Chung', ulf.hansson, linux-mmc
In-Reply-To: <e99052e2-de80-57a2-30ae-0baea2328cce@samsung.com>
Hi,
Yes, right. "include/uapi/linux/mmc/ioctl.h"
-----Original Message-----
From: linux-mmc-owner@vger.kernel.org
[mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of Jaehoon Chung
Sent: Tuesday, October 18, 2016 7:52 PM
To: Jeremy Kim; ulf.hansson@linaro.org; linux-mmc@vger.kernel.org
Subject: Re: [PATCH] mmc: block: Change MMC_IOC_MAX_BYTES
Hi,
On 10/18/2016 07:39 PM, Jeremy Kim wrote:
>
> 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>
> ---
> ioctl.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Where is ioctl.h? "include/uapi/linux/mmc/ioctl.h"?
Best Regards,
Jaehoon Chung
>
> diff --git a/ioctl.h b/ioctl.h
> index 7e385b8..700a551 100644
> --- a/ioctl.h
> +++ b/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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc"
> in the body of a message to majordomo@vger.kernel.org More majordomo
> info at http://vger.kernel.org/majordomo-info.html
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the
body of a message to majordomo@vger.kernel.org More majordomo info at
http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] mmc: block: Change MMC_IOC_MAX_BYTES
From: Jaehoon Chung @ 2016-10-18 10:51 UTC (permalink / raw)
To: Jeremy Kim, ulf.hansson, linux-mmc
In-Reply-To: <0bb501d2292b$e6a43760$b3eca620$@samsung.com>
Hi,
On 10/18/2016 07:39 PM, Jeremy Kim wrote:
>
> 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>
> ---
> ioctl.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Where is ioctl.h? "include/uapi/linux/mmc/ioctl.h"?
Best Regards,
Jaehoon Chung
>
> diff --git a/ioctl.h b/ioctl.h
> index 7e385b8..700a551 100644
> --- a/ioctl.h
> +++ b/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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
^ permalink raw reply
* Re: [v12, 0/8] Fix eSDHC host version register bug
From: Ulf Hansson @ 2016-10-18 10:47 UTC (permalink / raw)
To: Yangbo Lu
Cc: Mark Rutland, Xiaobo Xie, Minghuan Lian,
linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-clk,
Qiang Zhao, Russell King, Bhupesh Sharma, Jochen Friedrich,
Claudiu Manoil,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Arnd Bergmann,
Scott Wood, Rob Herring, Santosh Shilimkar,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA, linux-mmc,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Leo Li,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Kumar
In-Reply-To: <1474441040-11946-1-git-send-email-yangbo.lu-3arQi8VN3Tc@public.gmane.org>
On 21 September 2016 at 08:57, Yangbo Lu <yangbo.lu-3arQi8VN3Tc@public.gmane.org> wrote:
> This patchset is used to fix a host version register bug in the T4240-R1.0-R2.0
> eSDHC controller. To match the SoC version and revision, 10 previous version
> patchsets had tried many methods but all of them were rejected by reviewers.
> Such as
> - dts compatible method
> - syscon method
> - ifdef PPC method
> - GUTS driver getting SVR method
> Anrd suggested a soc_device_match method in v10, and this is the only available
> method left now. This v11 patchset introduces the soc_device_match interface in
> soc driver.
>
> The first six patches of Yangbo are to add the GUTS driver. This is used to
> register a soc device which contain soc version and revision information.
> The other two patches introduce the soc_device_match method in soc driver
> and apply it on esdhc driver to fix this bug.
>
> Arnd Bergmann (1):
> base: soc: introduce soc_device_match() interface
>
> Yangbo Lu (7):
> dt: bindings: update Freescale DCFG compatible
> ARM64: dts: ls2080a: add device configuration node
> dt: bindings: move guts devicetree doc out of powerpc directory
> powerpc/fsl: move mpc85xx.h to include/linux/fsl
> soc: fsl: add GUTS driver for QorIQ platforms
> MAINTAINERS: add entry for Freescale SoC drivers
> mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0
>
> Documentation/devicetree/bindings/arm/fsl.txt | 6 +-
> .../bindings/{powerpc => soc}/fsl/guts.txt | 3 +
> MAINTAINERS | 11 +-
> arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi | 6 +
> arch/powerpc/kernel/cpu_setup_fsl_booke.S | 2 +-
> arch/powerpc/sysdev/fsl_pci.c | 2 +-
> drivers/base/Kconfig | 1 +
> drivers/base/soc.c | 66 ++++++
> drivers/clk/clk-qoriq.c | 3 +-
> drivers/i2c/busses/i2c-mpc.c | 2 +-
> drivers/iommu/fsl_pamu.c | 3 +-
> drivers/mmc/host/Kconfig | 1 +
> drivers/mmc/host/sdhci-of-esdhc.c | 20 ++
> drivers/net/ethernet/freescale/gianfar.c | 2 +-
> drivers/soc/Kconfig | 2 +-
> drivers/soc/fsl/Kconfig | 19 ++
> drivers/soc/fsl/Makefile | 1 +
> drivers/soc/fsl/guts.c | 257 +++++++++++++++++++++
> include/linux/fsl/guts.h | 125 ++++++----
> .../asm/mpc85xx.h => include/linux/fsl/svr.h | 4 +-
> include/linux/sys_soc.h | 3 +
> 21 files changed, 478 insertions(+), 61 deletions(-)
> rename Documentation/devicetree/bindings/{powerpc => soc}/fsl/guts.txt (91%)
> create mode 100644 drivers/soc/fsl/Kconfig
> create mode 100644 drivers/soc/fsl/guts.c
> rename arch/powerpc/include/asm/mpc85xx.h => include/linux/fsl/svr.h (97%)
>
> --
> 2.1.0.27.g96db324
>
This looks good to me! I am not sure which tree you want this to be
picked up through, but unless no other volunteers I can take it
through my mmc tree.
Although, before considering to apply, I need an ack from Scott/Arnd
for the guts driver in patch 5/8 and I need an ack from Greg for patch
7/8, where the soc_device_match() interface is added (seems like you
didn't add him on cc/to).
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH v2 0/9] Init runtime PM support for dw_mmc
From: Jaehoon Chung @ 2016-10-18 10:45 UTC (permalink / raw)
To: Ulf Hansson, Shawn Lin
Cc: linux-mmc, Doug Anderson, open list:ARM/Rockchip SoC...,
Heiko Stuebner
In-Reply-To: <CAPDyKFopNgWh=+0RNFwJc+aaFPXGuJMAac5rPtfOawvLwSbN+g@mail.gmail.com>
On 10/18/2016 05:46 PM, Ulf Hansson wrote:
> + Heiko
>
> On 12 October 2016 at 04:50, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>
>> Hi Jaehoon and Ulf,
>>
>> This patch is gonna support runtime PM for dw_mmc.
>> It could support to disable ciu_clk by default and disable
>> biu_clk if the devices are non-removeable, or removeable
>> with gpio-base card detect.
>>
>> Then I remove the system PM since the runtime PM actually
>> does the same thing as it. So I help migrate the dw_mmc variant
>> drivers to use runtime PM pairs and pm_runtime_force_*. Note
>> that I only enable runtime PM for dw_mmc-rockchip as I will
>> leave the decision to the owners of the corresponding drivers.
>> I just tested it on my RK3288 platform with linux-next to make
>> the runtime PM and system PM work fine for my emmc, sd card and
>> sdio. But I don't have hardware to help test other variant drivers.
>> But in theory it should work fine as I mentioned that the runtime
>> PM does the same thing as system PM except for disabling ciu_clk
>> aggressively which should not be related to the variant hosts.
>>
>> As you could see that I just extend the slot-gpio a bit, so the
>> ideal way is Ulf could pick them up with Jaehoon's ack. :)
>
> The mmc core change looks fine to me, so I will wait for a pull
> request from Jaehoon.
>
>>
>>
>> Changes in v2:
>> - use struct device as argument for runtime callback
>> - use dw_mci_runtime_* directly
>> - use dw_mci_runtime_* directly
>> - minor fix since I change the argument for dw_mci_runtime_*
>> - use dw_mci_runtime_* directly
>> - use dw_mci_runtime_* directly
>>
>> 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(-)
>>
>
> Overall these changes looks good to me, so I am ready to accept the PR
> from Jaehoon!!
>
Yes..I will apply on my repository..i'm testing on dwmmc-testing branch..
Actually, i found the some blocking issue but it's not relevant to these patches.
after checking the stress test until today..(eMMC/SD/SDIO)..I will do.
Entire patches looks good to me..thanks for Shawn's effort! :)
>
> Although, highly related to this patchset, I am worried that there is
> a misunderstanding on how MMC_PM_KEEP_POWER (DT binding
> "keep-power-in-suspend") is being used for dw_mmc. Perhaps I am wrong,
> but I would appreciate if you could elaborate a bit for my
> understanding.
>
> First, this cap is solely intended to be used for controllers which
> may have SDIO cards attached, as it indicates those cards may be
> configured to be powered on while the system enters suspend state. By
> looking at some DTS files, for example
> arch/arm64/boot/dts/rockchip/rk3368-orion-r68-meta.dts which uses it
> for an eMMC slot, this is clearly being abused.
Right..it doesn't need to uses "keep-power-in-suspend" for eMMC/SD.
>
> Anyway, the wrong DT configurations might not be a big deal, as in
> dw_mci_resume(), it's not the capabilities bit that is checked but the
> corresponding "pm_flag". This flag is set via calling
> sdio_set_host_pm_flags(), but as that doesn't happen for an eMMC card
> we should be fine, right!?
>
> Now, what also do puzzles me, is exactly that piece of code in
> dw_mci_resume() that is being executed *when* the pm_flag contains
> MMC_PM_KEEP_POWER:
> if (slot->mmc->pm_flags & MMC_PM_KEEP_POWER) {
> dw_mci_set_ios(slot->mmc, &slot->mmc->ios);
> dw_mci_setup_bus(slot, true);
> }
There is no place to set this in dw_mmc controller..Maybe it should be located in other vendor driver.
Almost all cards that used SDIO might be WiFi..Of course..there might be other cards.
e.g) In exnyos, use the sdio as WiFi card(broadcom chipset) then broadcom driver also used..
When broadcom driver initialized or suspending time, it should be set to this flag..
As i mentioned..it's not normal case.
>
> So, in the system resume path, the above do makes sense as you need to
> restore the registers etc for the dw_mmc controller to enable it to
> operate the SDIO card. Such as bus width, clocks, etc.
>
> Although, I would expect something similar would be needed in the new
> runtime resume path as well. And in particular also for eMMC/SD cards,
> as you need to restore the dw_mmc registers to be able to operate the
> card again. Don't you?
Need to check :)
>
> So in the end, perhaps you should *always* call dw_mci_set_ios() and
> dw_mci_setup_bus() in dw_mci_resume() instead of conditionally check
> for MMC_PM_KEEP_POWER? Or maybe only a subset of those functions?
It's reasonable..but it also needs to check on real case and stress test.
Best Regards,
Jaehoon Chung
>
> Kind regards
> Uffe
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
^ permalink raw reply
* [PATCH] mmc: block: Change MMC_IOC_MAX_BYTES
From: Jeremy Kim @ 2016-10-18 10:39 UTC (permalink / raw)
To: ulf.hansson, linux-mmc; +Cc: jh4u.kim
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>
---
ioctl.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ioctl.h b/ioctl.h
index 7e385b8..700a551 100644
--- a/ioctl.h
+++ b/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 related
* Re: [PATCH] mmc: core: Check regulator pointer
From: Ulf Hansson @ 2016-10-18 10:28 UTC (permalink / raw)
To: Maxime Ripard; +Cc: linux-mmc, linux-kernel@vger.kernel.org
In-Reply-To: <20161018094704.ohubpg4j3jhc7cay@lukather>
On 18 October 2016 at 11:47, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi,
>
> On Tue, Oct 18, 2016 at 11:03:02AM +0200, Ulf Hansson wrote:
>> On 18 October 2016 at 10:43, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>> > mmc_regulator_get_supply might silently fail if the regulators are not
>> > found, which is the right thing to do since both these regulators are
>> > optional.
>> >
>> > However, the drivers then have no way to know whether or not they should
>> > proceed and call mmc_regulator_set_ocr. And since this function doesn't
>>
>> Host drivers should check "if (!IS_ERR(mmc->supply.vmmc))" before
>> invoking mmc_regulator_set_ocr(). I wasn't aware that some didn't.
>
> Ok, so the sunxi one definitely doesn't:
> http://lxr.free-electrons.com/source/drivers/mmc/host/sunxi-mmc.c#L735
>
>> My point is, that in some cases the regulator is optional, then a host
>> driver need to take other actions to power on/off the card.
>
> What are those actions? Just power up the card through some other
> mean, or is it more tied to the MMC protocol?
Through some other way, which is managed by the internal logic of the
mmc controller.
>
> Also, I'm wondering if VMMC is actually optional at all. In all cases
> I can think of, it would be represented as a regulator anyway.
Yes, you are right. For most cases, but not all.
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH v3 1/2] MMC: meson: initial support for GXBB platforms
From: Ulf Hansson @ 2016-10-18 10:25 UTC (permalink / raw)
To: Kevin Hilman
Cc: linux-mmc, linux-amlogic, linux-arm-kernel@lists.infradead.org,
Rob Herring, devicetree@vger.kernel.org, Victor Wan, Jerry Cao,
Xing Wu, yonghui.yu
In-Reply-To: <20160914004314.682-2-khilman@baylibre.com>
[...]
> +static int meson_mmc_clk_set(struct meson_host *host, unsigned long clk_rate)
> +{
> + struct mmc_host *mmc = host->mmc;
> + int ret = 0;
> + u32 cfg;
> +
> + if (clk_rate) {
> + if (WARN_ON(clk_rate > mmc->f_max))
> + clk_rate = mmc->f_max;
> + else if (WARN_ON(clk_rate < mmc->f_min))
> + clk_rate = mmc->f_min;
> + }
> +
> + if (clk_rate == mmc->actual_clock)
> + return 0;
> +
> + /* stop clock */
> + cfg = readl(host->regs + SD_EMMC_CFG);
> + if (!(cfg & CFG_STOP_CLOCK)) {
> + cfg |= CFG_STOP_CLOCK;
> + writel(cfg, host->regs + SD_EMMC_CFG);
> + }
> +
> + dev_dbg(host->dev, "change clock rate %u -> %lu\n",
> + mmc->actual_clock, clk_rate);
> + ret = clk_set_rate(host->cfg_div_clk, clk_rate);
Shouldn't you check the error before proceeding?
> + if (clk_rate && clk_rate != clk_get_rate(host->cfg_div_clk))
> + dev_warn(host->dev, "divider requested rate %lu != actual rate %lu: ret=%d\n",
> + clk_rate, clk_get_rate(host->cfg_div_clk), ret);
> + else
> + mmc->actual_clock = clk_rate;
> +
> + /* (re)start clock, if non-zero */
> + if (clk_rate) {
> + cfg = readl(host->regs + SD_EMMC_CFG);
> + cfg &= ~CFG_STOP_CLOCK;
> + writel(cfg, host->regs + SD_EMMC_CFG);
> + }
> +
> + return ret;
> +}
> +
> +/*
> + * The SD/eMMC IP block has an internal mux and divider used for
> + * generating the MMC clock. Use the clock framework to create and
> + * manage these clocks.
> + */
> +static int meson_mmc_clk_init(struct meson_host *host)
> +{
> + struct clk_init_data init;
> + char clk_name[32];
> + int i, ret = 0;
> + const char *mux_parent_names[MUX_CLK_NUM_PARENTS];
> + unsigned int mux_parent_count = 0;
> + const char *clk_div_parents[1];
> + unsigned int f_min = UINT_MAX;
> + u32 clk_reg, cfg;
> +
> + /* get the mux parents */
> + for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
> + char name[16];
> +
> + snprintf(name, sizeof(name), "clkin%d", i);
> + host->mux_parent[i] = devm_clk_get(host->dev, name);
> + if (IS_ERR(host->mux_parent[i])) {
> + ret = PTR_ERR(host->mux_parent[i]);
> + if (PTR_ERR(host->mux_parent[i]) != -EPROBE_DEFER)
> + dev_err(host->dev, "Missing clock %s\n", name);
> + host->mux_parent[i] = NULL;
> + return ret;
> + }
> +
> + host->mux_parent_rate[i] = clk_get_rate(host->mux_parent[i]);
> + mux_parent_names[i] = __clk_get_name(host->mux_parent[i]);
> + mux_parent_count++;
> + if (host->mux_parent_rate[i] < f_min)
> + f_min = host->mux_parent_rate[i];
> + }
> +
> + /* cacluate f_min based on input clocks, and max divider value */
> + if (f_min != UINT_MAX)
> + f_min = DIV_ROUND_UP(CLK_SRC_XTAL_RATE, CLK_DIV_MAX);
> + else
> + f_min = 4000000; /* default min: 400 MHz */
> + host->mmc->f_min = f_min;
> +
> + /* create the mux */
> + snprintf(clk_name, sizeof(clk_name), "%s#mux", dev_name(host->dev));
> + init.name = clk_name;
> + init.ops = &clk_mux_ops;
> + init.flags = 0;
> + init.parent_names = mux_parent_names;
> + init.num_parents = mux_parent_count;
> +
> + host->mux.reg = host->regs + SD_EMMC_CLOCK;
> + host->mux.shift = CLK_SRC_SHIFT;
> + host->mux.mask = CLK_SRC_MASK;
> + host->mux.flags = 0;
> + host->mux.table = NULL;
> + host->mux.hw.init = &init;
> +
> + host->mux_clk = devm_clk_register(host->dev, &host->mux.hw);
> + if (WARN_ON(IS_ERR(host->mux_clk)))
> + return PTR_ERR(host->mux_clk);
> +
> + /* create the divider */
> + snprintf(clk_name, sizeof(clk_name), "%s#div", dev_name(host->dev));
> + init.name = devm_kstrdup(host->dev, clk_name, GFP_KERNEL);
> + init.ops = &clk_divider_ops;
> + init.flags = CLK_SET_RATE_PARENT;
> + clk_div_parents[0] = __clk_get_name(host->mux_clk);
> + init.parent_names = clk_div_parents;
> + init.num_parents = ARRAY_SIZE(clk_div_parents);
> +
> + host->cfg_div.reg = host->regs + SD_EMMC_CLOCK;
> + host->cfg_div.shift = CLK_DIV_SHIFT;
> + host->cfg_div.width = CLK_DIV_WIDTH;
> + host->cfg_div.hw.init = &init;
> + host->cfg_div.flags = CLK_DIVIDER_ONE_BASED |
> + CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ALLOW_ZERO;
> +
> + host->cfg_div_clk = devm_clk_register(host->dev, &host->cfg_div.hw);
> + if (WARN_ON(PTR_ERR_OR_ZERO(host->cfg_div_clk)))
> + return PTR_ERR(host->cfg_div_clk);
> +
> + /* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
> + clk_reg = 0;
> + clk_reg |= CLK_PHASE_180 << CLK_PHASE_SHIFT;
> + clk_reg |= CLK_SRC_XTAL << CLK_SRC_SHIFT;
> + clk_reg |= CLK_DIV_MAX << CLK_DIV_SHIFT;
> + clk_reg &= ~CLK_ALWAYS_ON;
> + writel(clk_reg, host->regs + SD_EMMC_CLOCK);
> +
> + /* Ensure clock starts in "auto" mode, not "always on" */
> + cfg = readl(host->regs + SD_EMMC_CFG);
> + cfg &= ~CFG_CLK_ALWAYS_ON;
> + cfg |= CFG_AUTO_CLK;
> + writel(cfg, host->regs + SD_EMMC_CFG);
> +
> + ret = clk_prepare_enable(host->cfg_div_clk);
> + if (!ret)
> + ret = meson_mmc_clk_set(host, f_min);
> +
In case of errors, I guess you should disable/unprepare the clock.
> + return ret;
> +}
> +
[...]
> +
> +static int meson_mmc_probe(struct platform_device *pdev)
> +{
> + struct resource *res;
> + struct meson_host *host;
> + struct mmc_host *mmc;
> + int ret;
> +
> + mmc = mmc_alloc_host(sizeof(struct meson_host), &pdev->dev);
> + if (!mmc)
> + return -ENOMEM;
> + host = mmc_priv(mmc);
> + host->mmc = mmc;
> + host->dev = &pdev->dev;
> + dev_set_drvdata(&pdev->dev, host);
> +
> + spin_lock_init(&host->lock);
> +
> + host->core_clk = devm_clk_get(&pdev->dev, "core");
> + if (IS_ERR(host->core_clk)) {
> + ret = PTR_ERR(host->core_clk);
> + goto free_host;
> + }
> +
> + /* Get regulators and the supported OCR mask */
> + host->vqmmc_enabled = false;
> + ret = mmc_regulator_get_supply(mmc);
> + if (ret == -EPROBE_DEFER)
> + goto free_host;
In free_host you call clk_disable_unprepare(). You don't want to do
that unless you already called clk_prepare_enable().
So, I think you need another label to distinguish between these two cases.
> +
> + ret = mmc_of_parse(mmc);
> + if (ret) {
> + dev_warn(&pdev->dev, "error parsing DT: %d\n", ret);
> + goto free_host;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + host->regs = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(host->regs)) {
> + ret = PTR_ERR(host->regs);
> + goto free_host;
> + }
> +
> + host->irq = platform_get_irq(pdev, 0);
> + if (host->irq == 0) {
> + dev_err(&pdev->dev, "failed to get interrupt resource.\n");
> + ret = -EINVAL;
> + goto free_host;
> + }
> +
> + ret = clk_prepare_enable(host->core_clk);
> + if (ret)
> + goto free_host;
> +
> + ret = meson_mmc_clk_init(host);
> + if (ret)
> + goto free_host;
> +
> + /* Stop execution */
> + writel(0, host->regs + SD_EMMC_START);
> +
> + /* clear, ack, enable all interrupts */
> + writel(0, host->regs + SD_EMMC_IRQ_EN);
> + writel(IRQ_EN_MASK, host->regs + SD_EMMC_STATUS);
> +
> + ret = devm_request_threaded_irq(&pdev->dev, host->irq,
> + meson_mmc_irq, meson_mmc_irq_thread,
> + IRQF_SHARED, DRIVER_NAME, host);
> + if (ret)
> + goto free_host;
Besides the earlier issue mentiond for the label free_host, here you
also need to consider yet another clock that has be been enabled in
meson_mmc_clk_init(). I assume you want to disable this clock in the
error path.
> +
> + /* data bounce buffer */
> + host->bounce_buf_size = SZ_512K;
> + host->bounce_buf =
> + dma_alloc_coherent(host->dev, host->bounce_buf_size,
> + &host->bounce_dma_addr, GFP_KERNEL);
> + if (host->bounce_buf == NULL) {
> + dev_err(host->dev, "Unable to map allocate DMA bounce buffer.\n");
> + ret = -ENOMEM;
> + goto free_host;
> + }
> +
> + mmc->ops = &meson_mmc_ops;
> + mmc_add_host(mmc);
> +
> + return 0;
> +
> +free_host:
> + dev_dbg(host->dev, "Failed to probe: ret=%d\n", ret);
No need to print this, already managed by driver core.
> + if (host->core_clk)
No need to check above, already done by the clock framework.
> + clk_disable_unprepare(host->core_clk);
> + mmc_free_host(mmc);
> + return ret;
> +}
> +
> +static int meson_mmc_remove(struct platform_device *pdev)
> +{
> + struct meson_host *host = dev_get_drvdata(&pdev->dev);
> +
> + if (WARN_ON(!host))
> + return 0;
> +
> + if (host->bounce_buf)
> + dma_free_coherent(host->dev, host->bounce_buf_size,
> + host->bounce_buf, host->bounce_dma_addr);
> +
> + if (host->cfg_div_clk)
No need to check this, aleady done by the clock framework.
> + clk_disable_unprepare(host->cfg_div_clk);
> +
> + if (host->core_clk)
Ditto.
> + clk_disable_unprepare(host->core_clk);
> +
> + mmc_free_host(host->mmc);
> + return 0;
> +}
> +
[...]
Besides the minor stuff pointed out above, this looks great to me!
Before you re-spin, please drop this series from your branch which is
being integrated into linux-next, as otherwise I can't pick it up.
Kind regards
Uffe
^ permalink raw reply
* [PATCH v4] mmc: sdhci-msm: Add pm_runtime and system PM support
From: Pramod Gurav @ 2016-10-18 10:16 UTC (permalink / raw)
To: ulf.hansson, adrian.hunter, linux-mmc
Cc: linux-kernel, linux-arm-msm, linux-pm, riteshh, Pramod Gurav
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.
Signed-off-by: Pramod Gurav <pramod.gurav@linaro.org>
---
Tested on DB410C.
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 | 66 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 65 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 8ef44a2a..33ec809 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"
@@ -658,12 +659,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, 50);
+ pm_runtime_use_autosuspend(&pdev->dev);
+
ret = sdhci_add_host(host);
if (ret)
- goto clk_disable;
+ goto pm_runtime_disable;
+
+ platform_set_drvdata(pdev, host);
+
+ 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 +700,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 +713,56 @@ 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: %d\n", ret);
+ return ret;
+ }
+ ret = clk_prepare_enable(msm_host->pclk);
+ if (ret) {
+ dev_err(dev, "clk_enable failed: %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 related
* Re: [PATCH] mmc: core: Check regulator pointer
From: Maxime Ripard @ 2016-10-18 9:47 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, linux-kernel@vger.kernel.org
In-Reply-To: <CAPDyKFquN_f1ZKW3G6ZsTnnGJa-7N4R4-RpF=E4PN+7OTrxhZg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1260 bytes --]
Hi,
On Tue, Oct 18, 2016 at 11:03:02AM +0200, Ulf Hansson wrote:
> On 18 October 2016 at 10:43, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > mmc_regulator_get_supply might silently fail if the regulators are not
> > found, which is the right thing to do since both these regulators are
> > optional.
> >
> > However, the drivers then have no way to know whether or not they should
> > proceed and call mmc_regulator_set_ocr. And since this function doesn't
>
> Host drivers should check "if (!IS_ERR(mmc->supply.vmmc))" before
> invoking mmc_regulator_set_ocr(). I wasn't aware that some didn't.
Ok, so the sunxi one definitely doesn't:
http://lxr.free-electrons.com/source/drivers/mmc/host/sunxi-mmc.c#L735
> My point is, that in some cases the regulator is optional, then a host
> driver need to take other actions to power on/off the card.
What are those actions? Just power up the card through some other
mean, or is it more tied to the MMC protocol?
Also, I'm wondering if VMMC is actually optional at all. In all cases
I can think of, it would be represented as a regulator anyway.
Thanks,
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH] mmc: sdhci-esdhc-imx: make sure usdhc clock enabled while doing suspend
From: Ulf Hansson @ 2016-10-18 9:18 UTC (permalink / raw)
To: Haibo Chen; +Cc: Adrian Hunter, Dong Aisheng, linux-mmc, Aisheng Dong
In-Reply-To: <1476776340-23718-1-git-send-email-haibo.chen@nxp.com>
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!?
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?
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
>
> 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: core: Check regulator pointer
From: Ulf Hansson @ 2016-10-18 9:03 UTC (permalink / raw)
To: Maxime Ripard; +Cc: linux-mmc, linux-kernel@vger.kernel.org
In-Reply-To: <20161018084343.680-1-maxime.ripard@free-electrons.com>
On 18 October 2016 at 10:43, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> mmc_regulator_get_supply might silently fail if the regulators are not
> found, which is the right thing to do since both these regulators are
> optional.
>
> However, the drivers then have no way to know whether or not they should
> proceed and call mmc_regulator_set_ocr. And since this function doesn't
Host drivers should check "if (!IS_ERR(mmc->supply.vmmc))" before
invoking mmc_regulator_set_ocr(). I wasn't aware that some didn't.
My point is, that in some cases the regulator is optional, then a host
driver need to take other actions to power on/off the card. So, I am
wondering whether adding these checks in mmc_regulator_set_ocr() is a
bit unnecessary, as the host drivers are already checking the
IS_ERR().
> check for the validity of the regulator pointer, it leads to a null pointer
> dereference. Add such a check to make sure everything works fine.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
> drivers/mmc/core/core.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 2553d903a82b..1d3ea5e1aa37 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1474,6 +1474,12 @@ int mmc_regulator_set_ocr(struct mmc_host *mmc,
> int result = 0;
> int min_uV, max_uV;
>
> + if (!supply)
> + return -EINVAL;
> +
> + if (IS_ERR(supply))
> + return PTR_ERR(supply);
> +
> if (vdd_bit) {
> mmc_ocrbitnum_to_vdd(vdd_bit, &min_uV, &max_uV);
>
> --
> 2.9.3
>
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH] mmc: core: Check regulator pointer
From: Baolin Wang @ 2016-10-18 8:55 UTC (permalink / raw)
To: Maxime Ripard; +Cc: Ulf Hansson, linux-mmc, LKML
In-Reply-To: <20161018084343.680-1-maxime.ripard@free-electrons.com>
Hi,
On 18 October 2016 at 16:43, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> mmc_regulator_get_supply might silently fail if the regulators are not
> found, which is the right thing to do since both these regulators are
> optional.
>
> However, the drivers then have no way to know whether or not they should
> proceed and call mmc_regulator_set_ocr. And since this function doesn't
> check for the validity of the regulator pointer, it leads to a null pointer
> dereference. Add such a check to make sure everything works fine.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
> drivers/mmc/core/core.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 2553d903a82b..1d3ea5e1aa37 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1474,6 +1474,12 @@ int mmc_regulator_set_ocr(struct mmc_host *mmc,
> int result = 0;
> int min_uV, max_uV;
>
> + if (!supply)
> + return -EINVAL;
> +
> + if (IS_ERR(supply))
> + return PTR_ERR(supply);
You can simply these checking with IS_ERR_OR_NULL().
> +
> if (vdd_bit) {
> mmc_ocrbitnum_to_vdd(vdd_bit, &min_uV, &max_uV);
>
> --
> 2.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Baolin.wang
Best Regards
^ permalink raw reply
* Re: [PATCH v2 0/9] Init runtime PM support for dw_mmc
From: Ulf Hansson @ 2016-10-18 8:46 UTC (permalink / raw)
To: Shawn Lin
Cc: Jaehoon Chung, linux-mmc, Doug Anderson,
open list:ARM/Rockchip SoC..., Heiko Stuebner
In-Reply-To: <1476240643-5701-1-git-send-email-shawn.lin@rock-chips.com>
+ Heiko
On 12 October 2016 at 04:50, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> Hi Jaehoon and Ulf,
>
> This patch is gonna support runtime PM for dw_mmc.
> It could support to disable ciu_clk by default and disable
> biu_clk if the devices are non-removeable, or removeable
> with gpio-base card detect.
>
> Then I remove the system PM since the runtime PM actually
> does the same thing as it. So I help migrate the dw_mmc variant
> drivers to use runtime PM pairs and pm_runtime_force_*. Note
> that I only enable runtime PM for dw_mmc-rockchip as I will
> leave the decision to the owners of the corresponding drivers.
> I just tested it on my RK3288 platform with linux-next to make
> the runtime PM and system PM work fine for my emmc, sd card and
> sdio. But I don't have hardware to help test other variant drivers.
> But in theory it should work fine as I mentioned that the runtime
> PM does the same thing as system PM except for disabling ciu_clk
> aggressively which should not be related to the variant hosts.
>
> As you could see that I just extend the slot-gpio a bit, so the
> ideal way is Ulf could pick them up with Jaehoon's ack. :)
The mmc core change looks fine to me, so I will wait for a pull
request from Jaehoon.
>
>
> Changes in v2:
> - use struct device as argument for runtime callback
> - use dw_mci_runtime_* directly
> - use dw_mci_runtime_* directly
> - minor fix since I change the argument for dw_mci_runtime_*
> - use dw_mci_runtime_* directly
> - use dw_mci_runtime_* directly
>
> 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(-)
>
Overall these changes looks good to me, so I am ready to accept the PR
from Jaehoon!!
Although, highly related to this patchset, I am worried that there is
a misunderstanding on how MMC_PM_KEEP_POWER (DT binding
"keep-power-in-suspend") is being used for dw_mmc. Perhaps I am wrong,
but I would appreciate if you could elaborate a bit for my
understanding.
First, this cap is solely intended to be used for controllers which
may have SDIO cards attached, as it indicates those cards may be
configured to be powered on while the system enters suspend state. By
looking at some DTS files, for example
arch/arm64/boot/dts/rockchip/rk3368-orion-r68-meta.dts which uses it
for an eMMC slot, this is clearly being abused.
Anyway, the wrong DT configurations might not be a big deal, as in
dw_mci_resume(), it's not the capabilities bit that is checked but the
corresponding "pm_flag". This flag is set via calling
sdio_set_host_pm_flags(), but as that doesn't happen for an eMMC card
we should be fine, right!?
Now, what also do puzzles me, is exactly that piece of code in
dw_mci_resume() that is being executed *when* the pm_flag contains
MMC_PM_KEEP_POWER:
if (slot->mmc->pm_flags & MMC_PM_KEEP_POWER) {
dw_mci_set_ios(slot->mmc, &slot->mmc->ios);
dw_mci_setup_bus(slot, true);
}
So, in the system resume path, the above do makes sense as you need to
restore the registers etc for the dw_mmc controller to enable it to
operate the SDIO card. Such as bus width, clocks, etc.
Although, I would expect something similar would be needed in the new
runtime resume path as well. And in particular also for eMMC/SD cards,
as you need to restore the dw_mmc registers to be able to operate the
card again. Don't you?
So in the end, perhaps you should *always* call dw_mci_set_ios() and
dw_mci_setup_bus() in dw_mci_resume() instead of conditionally check
for MMC_PM_KEEP_POWER? Or maybe only a subset of those functions?
Kind regards
Uffe
^ permalink raw reply
* [PATCH] mmc: core: Check regulator pointer
From: Maxime Ripard @ 2016-10-18 8:43 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, linux-kernel, Maxime Ripard
mmc_regulator_get_supply might silently fail if the regulators are not
found, which is the right thing to do since both these regulators are
optional.
However, the drivers then have no way to know whether or not they should
proceed and call mmc_regulator_set_ocr. And since this function doesn't
check for the validity of the regulator pointer, it leads to a null pointer
dereference. Add such a check to make sure everything works fine.
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
drivers/mmc/core/core.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 2553d903a82b..1d3ea5e1aa37 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1474,6 +1474,12 @@ int mmc_regulator_set_ocr(struct mmc_host *mmc,
int result = 0;
int min_uV, max_uV;
+ if (!supply)
+ return -EINVAL;
+
+ if (IS_ERR(supply))
+ return PTR_ERR(supply);
+
if (vdd_bit) {
mmc_ocrbitnum_to_vdd(vdd_bit, &min_uV, &max_uV);
--
2.9.3
^ permalink raw reply related
* Re: Regression after "do not use CMD13 to get status after speed mode switch"
From: Linus Walleij @ 2016-10-18 8:36 UTC (permalink / raw)
To: linux-mmc@vger.kernel.org, Ulf Hansson, Chaotian Jing
Cc: linux-arm-msm@vger.kernel.org, Bjorn Andersson, Stephen Boyd,
Andy Gross
In-Reply-To: <CACRpkdbfNq+R9U9AwsKBM=xyKKTzyVDkiW4jqJ0Nb2LXvq20BA@mail.gmail.com>
On Mon, Oct 17, 2016 at 4:32 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> Before this patch the eMMC is detected and all partitions enumerated
> immediately, but after the patch it doesn't come up at all, except
> sometimes, when it appears minutes (!) after boot, all of a sudden.
FYI this is what it looks like when it eventually happens:
root@msm8660:/ [ 627.710175] mmc0: new high speed MMC card at address 0001
[ 627.711641] mmcblk0: mmc0:0001 SEM04G 3.69 GiB
[ 627.715485] mmcblk0boot0: mmc0:0001 SEM04G partition 1 1.00 MiB
[ 627.736654] mmcblk0boot1: mmc0:0001 SEM04G partition 2 1.00 MiB
[ 627.747397] mmcblk0rpmb: mmc0:0001 SEM04G partition 3 128 KiB
[ 627.756326] mmcblk0: p1 p2 p3 p4 < p5 p6 p7 p8 p9 p10 p11 p12 p13
p14 p15 p16 p17 p18 p19 p20 p21 >
So after 627 seconds, a bit hard for users to wait this long for their
root filesystem.
Yours,
Linus Walleij
^ 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