[PATCH 0/2] fix bug that cause tuning failure

[PATCH 0/2] fix bug that cause tuning failure

[Addy Ke]

Addy Ke (2):
  mmc: core: use card pointer as the first parameter of execute_tuning()
  mmc: dw_mmc: wait until card ready if tuning fails

 drivers/mmc/core/core.c           |  2 +-
 drivers/mmc/core/mmc_ops.h        |  1 -
 drivers/mmc/host/dw_mmc.c         | 51 +++++++++++++++++++++++++++++++++------
 drivers/mmc/host/rtsx_pci_sdmmc.c |  3 ++-
 drivers/mmc/host/rtsx_usb_sdmmc.c |  3 ++-
 include/linux/mmc/card.h          |  2 ++
 include/linux/mmc/host.h          |  2 +-
 7 files changed, 52 insertions(+), 12 deletions(-)

-- 
1.8.3.2

[PATCH 0/2] fix bug that cause tuning failure

[Addy Ke]

Addy Ke (2):
  mmc: core: use card pointer as the first parameter of execute_tuning()
  mmc: dw_mmc: wait until card ready if tuning fails

 drivers/mmc/core/core.c           |  2 +-
 drivers/mmc/core/mmc_ops.h        |  1 -
 drivers/mmc/host/dw_mmc.c         | 51 +++++++++++++++++++++++++++++++++------
 drivers/mmc/host/rtsx_pci_sdmmc.c |  3 ++-
 drivers/mmc/host/rtsx_usb_sdmmc.c |  3 ++-
 include/linux/mmc/card.h          |  2 ++
 include/linux/mmc/host.h          |  2 +-
 7 files changed, 52 insertions(+), 12 deletions(-)

-- 
1.8.3.2

[PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Addy Ke]

We need to take the card pointer in execute_tuning() for mmc_send_status(),
but mmc->card is NULL in tuning state. So we need change the first parameter
of execute_tuning() to card pointer(struct mmc_card * card).

Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
---
 drivers/mmc/core/core.c           | 2 +-
 drivers/mmc/host/dw_mmc.c         | 3 ++-
 drivers/mmc/host/rtsx_pci_sdmmc.c | 3 ++-
 drivers/mmc/host/rtsx_usb_sdmmc.c | 3 ++-
 include/linux/mmc/host.h          | 2 +-
 5 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 1be7055..271f024 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1101,7 +1101,7 @@ int mmc_execute_tuning(struct mmc_card *card)
 		opcode = MMC_SEND_TUNING_BLOCK;
 
 	mmc_host_clk_hold(host);
-	err = host->ops->execute_tuning(host, opcode);
+	err = host->ops->execute_tuning(card, opcode);
 	mmc_host_clk_release(host);
 
 	if (err)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 4bd22af..e54e656 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1485,8 +1485,9 @@ free_blk_test:
 	return ret;
 }
 
-static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode)
+static int dw_mci_execute_tuning(struct mmc_card *card, u32 opcode)
 {
+	struct mmc_host *mmc = card->host;
 	struct dw_mci_slot *slot = mmc_priv(mmc);
 	struct dw_mci *host = slot->host;
 	const struct dw_mci_drv_data *drv_data = host->drv_data;
diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c b/drivers/mmc/host/rtsx_pci_sdmmc.c
index 1d3d6c4..230bd2f 100644
--- a/drivers/mmc/host/rtsx_pci_sdmmc.c
+++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
@@ -1270,8 +1270,9 @@ out:
 	return err;
 }
 
-static int sdmmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
+static int sdmmc_execute_tuning(struct mmc_card *card, u32 opcode)
 {
+	struct mmc_host *mmc = card->host;
 	struct realtek_pci_sdmmc *host = mmc_priv(mmc);
 	struct rtsx_pcr *pcr = host->pcr;
 	int err = 0;
diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c b/drivers/mmc/host/rtsx_usb_sdmmc.c
index 88af827..c494c06 100644
--- a/drivers/mmc/host/rtsx_usb_sdmmc.c
+++ b/drivers/mmc/host/rtsx_usb_sdmmc.c
@@ -1265,8 +1265,9 @@ out:
 		return 0;
 }
 
-static int sdmmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
+static int sdmmc_execute_tuning(struct mmc_card *card, u32 opcode)
 {
+	struct mmc_host *mmc = card->host;
 	struct rtsx_usb_sdmmc *host = mmc_priv(mmc);
 	struct rtsx_ucr *ucr = host->ucr;
 	int err = 0;
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 0c8cbe5..ec4128e 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -133,7 +133,7 @@ struct mmc_host_ops {
 	int	(*card_busy)(struct mmc_host *host);
 
 	/* The tuning command opcode value is different for SD and eMMC cards */
-	int	(*execute_tuning)(struct mmc_host *host, u32 opcode);
+	int	(*execute_tuning)(struct mmc_card *card, u32 opcode);
 
 	/* Prepare HS400 target operating frequency depending host driver */
 	int	(*prepare_hs400_tuning)(struct mmc_host *host, struct mmc_ios *ios);
-- 
1.8.3.2

[PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Addy Ke]

We need to take the card pointer in execute_tuning() for mmc_send_status(),
but mmc->card is NULL in tuning state. So we need change the first parameter
of execute_tuning() to card pointer(struct mmc_card * card).

Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
---
 drivers/mmc/core/core.c           | 2 +-
 drivers/mmc/host/dw_mmc.c         | 3 ++-
 drivers/mmc/host/rtsx_pci_sdmmc.c | 3 ++-
 drivers/mmc/host/rtsx_usb_sdmmc.c | 3 ++-
 include/linux/mmc/host.h          | 2 +-
 5 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 1be7055..271f024 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1101,7 +1101,7 @@ int mmc_execute_tuning(struct mmc_card *card)
 		opcode = MMC_SEND_TUNING_BLOCK;
 
 	mmc_host_clk_hold(host);
-	err = host->ops->execute_tuning(host, opcode);
+	err = host->ops->execute_tuning(card, opcode);
 	mmc_host_clk_release(host);
 
 	if (err)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 4bd22af..e54e656 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1485,8 +1485,9 @@ free_blk_test:
 	return ret;
 }
 
-static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode)
+static int dw_mci_execute_tuning(struct mmc_card *card, u32 opcode)
 {
+	struct mmc_host *mmc = card->host;
 	struct dw_mci_slot *slot = mmc_priv(mmc);
 	struct dw_mci *host = slot->host;
 	const struct dw_mci_drv_data *drv_data = host->drv_data;
diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c b/drivers/mmc/host/rtsx_pci_sdmmc.c
index 1d3d6c4..230bd2f 100644
--- a/drivers/mmc/host/rtsx_pci_sdmmc.c
+++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
@@ -1270,8 +1270,9 @@ out:
 	return err;
 }
 
-static int sdmmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
+static int sdmmc_execute_tuning(struct mmc_card *card, u32 opcode)
 {
+	struct mmc_host *mmc = card->host;
 	struct realtek_pci_sdmmc *host = mmc_priv(mmc);
 	struct rtsx_pcr *pcr = host->pcr;
 	int err = 0;
diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c b/drivers/mmc/host/rtsx_usb_sdmmc.c
index 88af827..c494c06 100644
--- a/drivers/mmc/host/rtsx_usb_sdmmc.c
+++ b/drivers/mmc/host/rtsx_usb_sdmmc.c
@@ -1265,8 +1265,9 @@ out:
 		return 0;
 }
 
-static int sdmmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
+static int sdmmc_execute_tuning(struct mmc_card *card, u32 opcode)
 {
+	struct mmc_host *mmc = card->host;
 	struct rtsx_usb_sdmmc *host = mmc_priv(mmc);
 	struct rtsx_ucr *ucr = host->ucr;
 	int err = 0;
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 0c8cbe5..ec4128e 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -133,7 +133,7 @@ struct mmc_host_ops {
 	int	(*card_busy)(struct mmc_host *host);
 
 	/* The tuning command opcode value is different for SD and eMMC cards */
-	int	(*execute_tuning)(struct mmc_host *host, u32 opcode);
+	int	(*execute_tuning)(struct mmc_card *card, u32 opcode);
 
 	/* Prepare HS400 target operating frequency depending host driver */
 	int	(*prepare_hs400_tuning)(struct mmc_host *host, struct mmc_ios *ios);
-- 
1.8.3.2

[PATCH 2/2] mmc: dw_mmc: wait until card ready if tuning fails

[Addy Ke]

This patch based on Alex's patch:
https://patchwork.kernel.org/patch/5516411/

Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
---
 drivers/mmc/core/mmc_ops.h |  1 -
 drivers/mmc/host/dw_mmc.c  | 48 ++++++++++++++++++++++++++++++++++++++++------
 include/linux/mmc/card.h   |  2 ++
 3 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
index 6f4b00e..c5be9ce 100644
--- a/drivers/mmc/core/mmc_ops.h
+++ b/drivers/mmc/core/mmc_ops.h
@@ -20,7 +20,6 @@ int mmc_send_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr);
 int mmc_all_send_cid(struct mmc_host *host, u32 *cid);
 int mmc_set_relative_addr(struct mmc_card *card);
 int mmc_send_csd(struct mmc_card *card, u32 *csd);
-int mmc_send_status(struct mmc_card *card, u32 *status);
 int mmc_send_cid(struct mmc_host *host, u32 *cid);
 int mmc_spi_read_ocr(struct mmc_host *host, int highcap, u32 *ocrp);
 int mmc_spi_set_crc(struct mmc_host *host, int use_crc);
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index e54e656..4a31a5e 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1317,10 +1317,38 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
 	spin_unlock_irqrestore(&host->irq_lock, irqflags);
 }
 
-static int dw_mci_tuning_test(struct dw_mci_slot *slot, u32 opcode,
-			       struct dw_mci_tuning_data *tuning_data,
-			       u8 *blk_test)
+static int dw_mci_card_busy(u32 status)
 {
+	return !(status & R1_READY_FOR_DATA) ||
+		(R1_CURRENT_STATE(status) == R1_STATE_PRG);
+}
+
+static int dw_mci_wait_for_card_ready(struct mmc_card *card)
+{
+	struct mmc_host *mmc = card->host;
+	struct dw_mci_slot *slot = mmc_priv(mmc);
+	struct dw_mci *host = slot->host;
+	int err = 0;
+	u32 status;
+
+	do {
+		err = mmc_send_status(card, &status);
+		if (err) {
+			dev_err(host->dev,
+				"Get card status fail in tuning state\n");
+			break;
+		}
+	} while (dw_mci_card_busy(status));
+
+	return err;
+}
+
+static int dw_mci_tuning_test(struct mmc_card *card, u32 opcode,
+			      struct dw_mci_tuning_data *tuning_data,
+			      u8 *blk_test)
+{
+	struct mmc_host *mmc = card->host;
+	struct dw_mci_slot *slot = mmc_priv(mmc);
 	struct dw_mci *host = slot->host;
 	struct mmc_host *mmc = slot->mmc;
 	const u8 *blk_pattern = tuning_data->blk_pattern;
@@ -1330,6 +1358,7 @@ static int dw_mci_tuning_test(struct dw_mci_slot *slot, u32 opcode,
 	struct mmc_command stop = {0};
 	struct mmc_data data = {0};
 	struct scatterlist sg;
+	int err;
 
 	memset(blk_test, 0, blksz);
 
@@ -1365,6 +1394,11 @@ static int dw_mci_tuning_test(struct dw_mci_slot *slot, u32 opcode,
 		dev_dbg(host->dev,
 			"Tuning error: cmd.error:%d, data.error:%d\n",
 			cmd.error, data.error);
+
+		err = dw_mci_wait_for_card_ready(card);
+		if (err)
+			return err;
+
 		if (cmd.error)
 			return cmd.error;
 		else
@@ -1372,9 +1406,11 @@ static int dw_mci_tuning_test(struct dw_mci_slot *slot, u32 opcode,
 	}
 }
 
-static int dw_mci_execute_generic_tuning(struct dw_mci_slot *slot, u32 opcode,
+static int dw_mci_execute_generic_tuning(struct mmc_card *card, u32 opcode,
 					 struct dw_mci_tuning_data *tuning_data)
 {
+	struct mmc_host *mmc = card->host;
+	struct dw_mci_slot *slot = mmc_priv(mmc);
 	struct dw_mci *host = slot->host;
 	unsigned int blksz = tuning_data->blksz;
 	u8 *blk_test;
@@ -1412,7 +1448,7 @@ static int dw_mci_execute_generic_tuning(struct dw_mci_slot *slot, u32 opcode,
 	for (i = 0; i < NUM_PHASES; i++) {
 		clk_set_phase(host->sample_clk, i * PHASE_INCREMENT);
 
-		v = !dw_mci_tuning_test(slot, opcode, tuning_data, blk_test);
+		v = !dw_mci_tuning_test(card, opcode, tuning_data, blk_test);
 
 		if ((!prev_v) && v) {
 			range_count++;
@@ -1496,7 +1532,7 @@ static int dw_mci_execute_tuning(struct mmc_card *card, u32 opcode)
 	if (drv_data && drv_data->execute_tuning)
 		err = drv_data->execute_tuning(slot, opcode, &tuning_data);
 	else
-		err = dw_mci_execute_generic_tuning(slot, opcode, &tuning_data);
+		err = dw_mci_execute_generic_tuning(card, opcode, &tuning_data);
 	return err;
 }
 
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 4d69c00..40d90ae 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -518,4 +518,6 @@ extern void mmc_unregister_driver(struct device_driver *);
 extern void mmc_fixup_device(struct mmc_card *card,
 			     const struct mmc_fixup *table);
 
+int mmc_send_status(struct mmc_card *card, u32 *status);
+
 #endif /* LINUX_MMC_CARD_H */
-- 
1.8.3.2

[PATCH 2/2] mmc: dw_mmc: wait until card ready if tuning fails

[Addy Ke]

This patch based on Alex's patch:
https://patchwork.kernel.org/patch/5516411/

Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
---
 drivers/mmc/core/mmc_ops.h |  1 -
 drivers/mmc/host/dw_mmc.c  | 48 ++++++++++++++++++++++++++++++++++++++++------
 include/linux/mmc/card.h   |  2 ++
 3 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
index 6f4b00e..c5be9ce 100644
--- a/drivers/mmc/core/mmc_ops.h
+++ b/drivers/mmc/core/mmc_ops.h
@@ -20,7 +20,6 @@ int mmc_send_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr);
 int mmc_all_send_cid(struct mmc_host *host, u32 *cid);
 int mmc_set_relative_addr(struct mmc_card *card);
 int mmc_send_csd(struct mmc_card *card, u32 *csd);
-int mmc_send_status(struct mmc_card *card, u32 *status);
 int mmc_send_cid(struct mmc_host *host, u32 *cid);
 int mmc_spi_read_ocr(struct mmc_host *host, int highcap, u32 *ocrp);
 int mmc_spi_set_crc(struct mmc_host *host, int use_crc);
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index e54e656..4a31a5e 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1317,10 +1317,38 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
 	spin_unlock_irqrestore(&host->irq_lock, irqflags);
 }
 
-static int dw_mci_tuning_test(struct dw_mci_slot *slot, u32 opcode,
-			       struct dw_mci_tuning_data *tuning_data,
-			       u8 *blk_test)
+static int dw_mci_card_busy(u32 status)
 {
+	return !(status & R1_READY_FOR_DATA) ||
+		(R1_CURRENT_STATE(status) == R1_STATE_PRG);
+}
+
+static int dw_mci_wait_for_card_ready(struct mmc_card *card)
+{
+	struct mmc_host *mmc = card->host;
+	struct dw_mci_slot *slot = mmc_priv(mmc);
+	struct dw_mci *host = slot->host;
+	int err = 0;
+	u32 status;
+
+	do {
+		err = mmc_send_status(card, &status);
+		if (err) {
+			dev_err(host->dev,
+				"Get card status fail in tuning state\n");
+			break;
+		}
+	} while (dw_mci_card_busy(status));
+
+	return err;
+}
+
+static int dw_mci_tuning_test(struct mmc_card *card, u32 opcode,
+			      struct dw_mci_tuning_data *tuning_data,
+			      u8 *blk_test)
+{
+	struct mmc_host *mmc = card->host;
+	struct dw_mci_slot *slot = mmc_priv(mmc);
 	struct dw_mci *host = slot->host;
 	struct mmc_host *mmc = slot->mmc;
 	const u8 *blk_pattern = tuning_data->blk_pattern;
@@ -1330,6 +1358,7 @@ static int dw_mci_tuning_test(struct dw_mci_slot *slot, u32 opcode,
 	struct mmc_command stop = {0};
 	struct mmc_data data = {0};
 	struct scatterlist sg;
+	int err;
 
 	memset(blk_test, 0, blksz);
 
@@ -1365,6 +1394,11 @@ static int dw_mci_tuning_test(struct dw_mci_slot *slot, u32 opcode,
 		dev_dbg(host->dev,
 			"Tuning error: cmd.error:%d, data.error:%d\n",
 			cmd.error, data.error);
+
+		err = dw_mci_wait_for_card_ready(card);
+		if (err)
+			return err;
+
 		if (cmd.error)
 			return cmd.error;
 		else
@@ -1372,9 +1406,11 @@ static int dw_mci_tuning_test(struct dw_mci_slot *slot, u32 opcode,
 	}
 }
 
-static int dw_mci_execute_generic_tuning(struct dw_mci_slot *slot, u32 opcode,
+static int dw_mci_execute_generic_tuning(struct mmc_card *card, u32 opcode,
 					 struct dw_mci_tuning_data *tuning_data)
 {
+	struct mmc_host *mmc = card->host;
+	struct dw_mci_slot *slot = mmc_priv(mmc);
 	struct dw_mci *host = slot->host;
 	unsigned int blksz = tuning_data->blksz;
 	u8 *blk_test;
@@ -1412,7 +1448,7 @@ static int dw_mci_execute_generic_tuning(struct dw_mci_slot *slot, u32 opcode,
 	for (i = 0; i < NUM_PHASES; i++) {
 		clk_set_phase(host->sample_clk, i * PHASE_INCREMENT);
 
-		v = !dw_mci_tuning_test(slot, opcode, tuning_data, blk_test);
+		v = !dw_mci_tuning_test(card, opcode, tuning_data, blk_test);
 
 		if ((!prev_v) && v) {
 			range_count++;
@@ -1496,7 +1532,7 @@ static int dw_mci_execute_tuning(struct mmc_card *card, u32 opcode)
 	if (drv_data && drv_data->execute_tuning)
 		err = drv_data->execute_tuning(slot, opcode, &tuning_data);
 	else
-		err = dw_mci_execute_generic_tuning(slot, opcode, &tuning_data);
+		err = dw_mci_execute_generic_tuning(card, opcode, &tuning_data);
 	return err;
 }
 
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 4d69c00..40d90ae 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -518,4 +518,6 @@ extern void mmc_unregister_driver(struct device_driver *);
 extern void mmc_fixup_device(struct mmc_card *card,
 			     const struct mmc_fixup *table);
 
+int mmc_send_status(struct mmc_card *card, u32 *status);
+
 #endif /* LINUX_MMC_CARD_H */
-- 
1.8.3.2

Re: [PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Ulf Hansson]

On 26 January 2015 at 12:19, Addy Ke <addy.ke@rock-chips.com> wrote:
> We need to take the card pointer in execute_tuning() for mmc_send_status(),

mmc_send_status() is an mmc core function, not intended for host's to call.

> but mmc->card is NULL in tuning state. So we need change the first parameter
> of execute_tuning() to card pointer(struct mmc_card * card).

So, why do we need this?

Kind regards
Uffe

>
> Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
> ---
>  drivers/mmc/core/core.c           | 2 +-
>  drivers/mmc/host/dw_mmc.c         | 3 ++-
>  drivers/mmc/host/rtsx_pci_sdmmc.c | 3 ++-
>  drivers/mmc/host/rtsx_usb_sdmmc.c | 3 ++-
>  include/linux/mmc/host.h          | 2 +-
>  5 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 1be7055..271f024 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1101,7 +1101,7 @@ int mmc_execute_tuning(struct mmc_card *card)
>                 opcode = MMC_SEND_TUNING_BLOCK;
>
>         mmc_host_clk_hold(host);
> -       err = host->ops->execute_tuning(host, opcode);
> +       err = host->ops->execute_tuning(card, opcode);
>         mmc_host_clk_release(host);
>
>         if (err)
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 4bd22af..e54e656 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1485,8 +1485,9 @@ free_blk_test:
>         return ret;
>  }
>
> -static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode)
> +static int dw_mci_execute_tuning(struct mmc_card *card, u32 opcode)
>  {
> +       struct mmc_host *mmc = card->host;
>         struct dw_mci_slot *slot = mmc_priv(mmc);
>         struct dw_mci *host = slot->host;
>         const struct dw_mci_drv_data *drv_data = host->drv_data;
> diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c b/drivers/mmc/host/rtsx_pci_sdmmc.c
> index 1d3d6c4..230bd2f 100644
> --- a/drivers/mmc/host/rtsx_pci_sdmmc.c
> +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
> @@ -1270,8 +1270,9 @@ out:
>         return err;
>  }
>
> -static int sdmmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
> +static int sdmmc_execute_tuning(struct mmc_card *card, u32 opcode)
>  {
> +       struct mmc_host *mmc = card->host;
>         struct realtek_pci_sdmmc *host = mmc_priv(mmc);
>         struct rtsx_pcr *pcr = host->pcr;
>         int err = 0;
> diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c b/drivers/mmc/host/rtsx_usb_sdmmc.c
> index 88af827..c494c06 100644
> --- a/drivers/mmc/host/rtsx_usb_sdmmc.c
> +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c
> @@ -1265,8 +1265,9 @@ out:
>                 return 0;
>  }
>
> -static int sdmmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
> +static int sdmmc_execute_tuning(struct mmc_card *card, u32 opcode)
>  {
> +       struct mmc_host *mmc = card->host;
>         struct rtsx_usb_sdmmc *host = mmc_priv(mmc);
>         struct rtsx_ucr *ucr = host->ucr;
>         int err = 0;
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 0c8cbe5..ec4128e 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -133,7 +133,7 @@ struct mmc_host_ops {
>         int     (*card_busy)(struct mmc_host *host);
>
>         /* The tuning command opcode value is different for SD and eMMC cards */
> -       int     (*execute_tuning)(struct mmc_host *host, u32 opcode);
> +       int     (*execute_tuning)(struct mmc_card *card, u32 opcode);
>
>         /* Prepare HS400 target operating frequency depending host driver */
>         int     (*prepare_hs400_tuning)(struct mmc_host *host, struct mmc_ios *ios);
> --
> 1.8.3.2
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Ulf Hansson]

On 26 January 2015 at 12:19, Addy Ke <addy.ke-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
> We need to take the card pointer in execute_tuning() for mmc_send_status(),

mmc_send_status() is an mmc core function, not intended for host's to call.

> but mmc->card is NULL in tuning state. So we need change the first parameter
> of execute_tuning() to card pointer(struct mmc_card * card).

So, why do we need this?

Kind regards
Uffe

>
> Signed-off-by: Addy Ke <addy.ke-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
>  drivers/mmc/core/core.c           | 2 +-
>  drivers/mmc/host/dw_mmc.c         | 3 ++-
>  drivers/mmc/host/rtsx_pci_sdmmc.c | 3 ++-
>  drivers/mmc/host/rtsx_usb_sdmmc.c | 3 ++-
>  include/linux/mmc/host.h          | 2 +-
>  5 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 1be7055..271f024 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1101,7 +1101,7 @@ int mmc_execute_tuning(struct mmc_card *card)
>                 opcode = MMC_SEND_TUNING_BLOCK;
>
>         mmc_host_clk_hold(host);
> -       err = host->ops->execute_tuning(host, opcode);
> +       err = host->ops->execute_tuning(card, opcode);
>         mmc_host_clk_release(host);
>
>         if (err)
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 4bd22af..e54e656 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1485,8 +1485,9 @@ free_blk_test:
>         return ret;
>  }
>
> -static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode)
> +static int dw_mci_execute_tuning(struct mmc_card *card, u32 opcode)
>  {
> +       struct mmc_host *mmc = card->host;
>         struct dw_mci_slot *slot = mmc_priv(mmc);
>         struct dw_mci *host = slot->host;
>         const struct dw_mci_drv_data *drv_data = host->drv_data;
> diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c b/drivers/mmc/host/rtsx_pci_sdmmc.c
> index 1d3d6c4..230bd2f 100644
> --- a/drivers/mmc/host/rtsx_pci_sdmmc.c
> +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
> @@ -1270,8 +1270,9 @@ out:
>         return err;
>  }
>
> -static int sdmmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
> +static int sdmmc_execute_tuning(struct mmc_card *card, u32 opcode)
>  {
> +       struct mmc_host *mmc = card->host;
>         struct realtek_pci_sdmmc *host = mmc_priv(mmc);
>         struct rtsx_pcr *pcr = host->pcr;
>         int err = 0;
> diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c b/drivers/mmc/host/rtsx_usb_sdmmc.c
> index 88af827..c494c06 100644
> --- a/drivers/mmc/host/rtsx_usb_sdmmc.c
> +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c
> @@ -1265,8 +1265,9 @@ out:
>                 return 0;
>  }
>
> -static int sdmmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
> +static int sdmmc_execute_tuning(struct mmc_card *card, u32 opcode)
>  {
> +       struct mmc_host *mmc = card->host;
>         struct rtsx_usb_sdmmc *host = mmc_priv(mmc);
>         struct rtsx_ucr *ucr = host->ucr;
>         int err = 0;
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 0c8cbe5..ec4128e 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -133,7 +133,7 @@ struct mmc_host_ops {
>         int     (*card_busy)(struct mmc_host *host);
>
>         /* The tuning command opcode value is different for SD and eMMC cards */
> -       int     (*execute_tuning)(struct mmc_host *host, u32 opcode);
> +       int     (*execute_tuning)(struct mmc_card *card, u32 opcode);
>
>         /* Prepare HS400 target operating frequency depending host driver */
>         int     (*prepare_hs400_tuning)(struct mmc_host *host, struct mmc_ios *ios);
> --
> 1.8.3.2
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Ulf Hansson]

On 26 January 2015 at 12:19, Addy Ke <addy.ke@rock-chips.com> wrote:
> We need to take the card pointer in execute_tuning() for mmc_send_status(),

mmc_send_status() is an mmc core function, not intended for host's to call.

> but mmc->card is NULL in tuning state. So we need change the first parameter
> of execute_tuning() to card pointer(struct mmc_card * card).

So, why do we need this?

Kind regards
Uffe

>
> Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
> ---
>  drivers/mmc/core/core.c           | 2 +-
>  drivers/mmc/host/dw_mmc.c         | 3 ++-
>  drivers/mmc/host/rtsx_pci_sdmmc.c | 3 ++-
>  drivers/mmc/host/rtsx_usb_sdmmc.c | 3 ++-
>  include/linux/mmc/host.h          | 2 +-
>  5 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 1be7055..271f024 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1101,7 +1101,7 @@ int mmc_execute_tuning(struct mmc_card *card)
>                 opcode = MMC_SEND_TUNING_BLOCK;
>
>         mmc_host_clk_hold(host);
> -       err = host->ops->execute_tuning(host, opcode);
> +       err = host->ops->execute_tuning(card, opcode);
>         mmc_host_clk_release(host);
>
>         if (err)
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 4bd22af..e54e656 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1485,8 +1485,9 @@ free_blk_test:
>         return ret;
>  }
>
> -static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode)
> +static int dw_mci_execute_tuning(struct mmc_card *card, u32 opcode)
>  {
> +       struct mmc_host *mmc = card->host;
>         struct dw_mci_slot *slot = mmc_priv(mmc);
>         struct dw_mci *host = slot->host;
>         const struct dw_mci_drv_data *drv_data = host->drv_data;
> diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c b/drivers/mmc/host/rtsx_pci_sdmmc.c
> index 1d3d6c4..230bd2f 100644
> --- a/drivers/mmc/host/rtsx_pci_sdmmc.c
> +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
> @@ -1270,8 +1270,9 @@ out:
>         return err;
>  }
>
> -static int sdmmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
> +static int sdmmc_execute_tuning(struct mmc_card *card, u32 opcode)
>  {
> +       struct mmc_host *mmc = card->host;
>         struct realtek_pci_sdmmc *host = mmc_priv(mmc);
>         struct rtsx_pcr *pcr = host->pcr;
>         int err = 0;
> diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c b/drivers/mmc/host/rtsx_usb_sdmmc.c
> index 88af827..c494c06 100644
> --- a/drivers/mmc/host/rtsx_usb_sdmmc.c
> +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c
> @@ -1265,8 +1265,9 @@ out:
>                 return 0;
>  }
>
> -static int sdmmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
> +static int sdmmc_execute_tuning(struct mmc_card *card, u32 opcode)
>  {
> +       struct mmc_host *mmc = card->host;
>         struct rtsx_usb_sdmmc *host = mmc_priv(mmc);
>         struct rtsx_ucr *ucr = host->ucr;
>         int err = 0;
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 0c8cbe5..ec4128e 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -133,7 +133,7 @@ struct mmc_host_ops {
>         int     (*card_busy)(struct mmc_host *host);
>
>         /* The tuning command opcode value is different for SD and eMMC cards */
> -       int     (*execute_tuning)(struct mmc_host *host, u32 opcode);
> +       int     (*execute_tuning)(struct mmc_card *card, u32 opcode);
>
>         /* Prepare HS400 target operating frequency depending host driver */
>         int     (*prepare_hs400_tuning)(struct mmc_host *host, struct mmc_ios *ios);
> --
> 1.8.3.2
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Doug Anderson]

Ulf,

On Mon, Jan 26, 2015 at 7:15 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 26 January 2015 at 12:19, Addy Ke <addy.ke@rock-chips.com> wrote:
>> We need to take the card pointer in execute_tuning() for mmc_send_status(),
>
> mmc_send_status() is an mmc core function, not intended for host's to call.
>
>> but mmc->card is NULL in tuning state. So we need change the first parameter
>> of execute_tuning() to card pointer(struct mmc_card * card).
>
> So, why do we need this?

I asked Addy to post upstream against mmc_send_tuning(), but I guess
he didn't (he posted against Alex's NAKed patch instead).

...when I talked to him about it, Addy was asserting that when tuning
fails it is important (at least on dw_mmc on rk3288) that we wait for
the card to stop being busy and that the way to detect was using
mmc_send_status().

That would mean that against upstream you'd need to change
mmc_send_tuning() to take in the card as well (or move the "host->card
= card" assignment to before UHS init, which seems less desirable?)

What do you think about that?  Is there a better solution?

-Doug

Re: [PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Doug Anderson]

Ulf,

On Mon, Jan 26, 2015 at 7:15 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 26 January 2015 at 12:19, Addy Ke <addy.ke@rock-chips.com> wrote:
>> We need to take the card pointer in execute_tuning() for mmc_send_status(),
>
> mmc_send_status() is an mmc core function, not intended for host's to call.
>
>> but mmc->card is NULL in tuning state. So we need change the first parameter
>> of execute_tuning() to card pointer(struct mmc_card * card).
>
> So, why do we need this?

I asked Addy to post upstream against mmc_send_tuning(), but I guess
he didn't (he posted against Alex's NAKed patch instead).

...when I talked to him about it, Addy was asserting that when tuning
fails it is important (at least on dw_mmc on rk3288) that we wait for
the card to stop being busy and that the way to detect was using
mmc_send_status().

That would mean that against upstream you'd need to change
mmc_send_tuning() to take in the card as well (or move the "host->card
= card" assignment to before UHS init, which seems less desirable?)

What do you think about that?  Is there a better solution?

-Doug

[PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Doug Anderson]

Ulf,

On Mon, Jan 26, 2015 at 7:15 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 26 January 2015 at 12:19, Addy Ke <addy.ke@rock-chips.com> wrote:
>> We need to take the card pointer in execute_tuning() for mmc_send_status(),
>
> mmc_send_status() is an mmc core function, not intended for host's to call.
>
>> but mmc->card is NULL in tuning state. So we need change the first parameter
>> of execute_tuning() to card pointer(struct mmc_card * card).
>
> So, why do we need this?

I asked Addy to post upstream against mmc_send_tuning(), but I guess
he didn't (he posted against Alex's NAKed patch instead).

...when I talked to him about it, Addy was asserting that when tuning
fails it is important (at least on dw_mmc on rk3288) that we wait for
the card to stop being busy and that the way to detect was using
mmc_send_status().

That would mean that against upstream you'd need to change
mmc_send_tuning() to take in the card as well (or move the "host->card
= card" assignment to before UHS init, which seems less desirable?)

What do you think about that?  Is there a better solution?

-Doug

Re: [PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Russell King - ARM Linux]

On Mon, Jan 26, 2015 at 09:45:07AM -0800, Doug Anderson wrote:
> Ulf,
> 
> On Mon, Jan 26, 2015 at 7:15 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On 26 January 2015 at 12:19, Addy Ke <addy.ke@rock-chips.com> wrote:
> >> We need to take the card pointer in execute_tuning() for mmc_send_status(),
> >
> > mmc_send_status() is an mmc core function, not intended for host's to call.
> >
> >> but mmc->card is NULL in tuning state. So we need change the first parameter
> >> of execute_tuning() to card pointer(struct mmc_card * card).
> >
> > So, why do we need this?
> 
> I asked Addy to post upstream against mmc_send_tuning(), but I guess
> he didn't (he posted against Alex's NAKed patch instead).
> 
> ...when I talked to him about it, Addy was asserting that when tuning
> fails it is important (at least on dw_mmc on rk3288) that we wait for
> the card to stop being busy and that the way to detect was using
> mmc_send_status().
> 
> That would mean that against upstream you'd need to change
> mmc_send_tuning() to take in the card as well (or move the "host->card
> = card" assignment to before UHS init, which seems less desirable?)
> 
> What do you think about that?  Is there a better solution?

That sounds like a generic thing though - in which case, what do the
specs have to say on this, and does the code implement what it says?

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Russell King - ARM Linux]

On Mon, Jan 26, 2015 at 09:45:07AM -0800, Doug Anderson wrote:
> Ulf,
> 
> On Mon, Jan 26, 2015 at 7:15 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On 26 January 2015 at 12:19, Addy Ke <addy.ke@rock-chips.com> wrote:
> >> We need to take the card pointer in execute_tuning() for mmc_send_status(),
> >
> > mmc_send_status() is an mmc core function, not intended for host's to call.
> >
> >> but mmc->card is NULL in tuning state. So we need change the first parameter
> >> of execute_tuning() to card pointer(struct mmc_card * card).
> >
> > So, why do we need this?
> 
> I asked Addy to post upstream against mmc_send_tuning(), but I guess
> he didn't (he posted against Alex's NAKed patch instead).
> 
> ...when I talked to him about it, Addy was asserting that when tuning
> fails it is important (at least on dw_mmc on rk3288) that we wait for
> the card to stop being busy and that the way to detect was using
> mmc_send_status().
> 
> That would mean that against upstream you'd need to change
> mmc_send_tuning() to take in the card as well (or move the "host->card
> = card" assignment to before UHS init, which seems less desirable?)
> 
> What do you think about that?  Is there a better solution?

That sounds like a generic thing though - in which case, what do the
specs have to say on this, and does the code implement what it says?

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Russell King - ARM Linux]

On Mon, Jan 26, 2015 at 09:45:07AM -0800, Doug Anderson wrote:
> Ulf,
> 
> On Mon, Jan 26, 2015 at 7:15 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On 26 January 2015 at 12:19, Addy Ke <addy.ke@rock-chips.com> wrote:
> >> We need to take the card pointer in execute_tuning() for mmc_send_status(),
> >
> > mmc_send_status() is an mmc core function, not intended for host's to call.
> >
> >> but mmc->card is NULL in tuning state. So we need change the first parameter
> >> of execute_tuning() to card pointer(struct mmc_card * card).
> >
> > So, why do we need this?
> 
> I asked Addy to post upstream against mmc_send_tuning(), but I guess
> he didn't (he posted against Alex's NAKed patch instead).
> 
> ...when I talked to him about it, Addy was asserting that when tuning
> fails it is important (at least on dw_mmc on rk3288) that we wait for
> the card to stop being busy and that the way to detect was using
> mmc_send_status().
> 
> That would mean that against upstream you'd need to change
> mmc_send_tuning() to take in the card as well (or move the "host->card
> = card" assignment to before UHS init, which seems less desirable?)
> 
> What do you think about that?  Is there a better solution?

That sounds like a generic thing though - in which case, what do the
specs have to say on this, and does the code implement what it says?

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Ulf Hansson]

On 26 January 2015 at 18:45, Doug Anderson <dianders@chromium.org> wrote:
> Ulf,
>
> On Mon, Jan 26, 2015 at 7:15 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 26 January 2015 at 12:19, Addy Ke <addy.ke@rock-chips.com> wrote:
>>> We need to take the card pointer in execute_tuning() for mmc_send_status(),
>>
>> mmc_send_status() is an mmc core function, not intended for host's to call.
>>
>>> but mmc->card is NULL in tuning state. So we need change the first parameter
>>> of execute_tuning() to card pointer(struct mmc_card * card).
>>
>> So, why do we need this?
>
> I asked Addy to post upstream against mmc_send_tuning(), but I guess
> he didn't (he posted against Alex's NAKed patch instead).
>
> ...when I talked to him about it, Addy was asserting that when tuning
> fails it is important (at least on dw_mmc on rk3288) that we wait for
> the card to stop being busy and that the way to detect was using
> mmc_send_status().

So, could that be due to the internal logic of the error handling in
dw_mmc driver? Or you think this is a generic issue?

According to the specifications (eMMC and SD) both states that the
tuning command has an R1 response. So, there shouldn't be any busy
signalling involved - at least according to spec.

>
> That would mean that against upstream you'd need to change
> mmc_send_tuning() to take in the card as well (or move the "host->card
> = card" assignment to before UHS init, which seems less desirable?)
>
> What do you think about that?  Is there a better solution?

Why do we need to change mmc_send_tuning()? I thought the issue was
that mmc_send_status(), which currently takes "card" as a parameter.

Kind regards
Uffe

Re: [PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Ulf Hansson]

On 26 January 2015 at 18:45, Doug Anderson <dianders@chromium.org> wrote:
> Ulf,
>
> On Mon, Jan 26, 2015 at 7:15 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 26 January 2015 at 12:19, Addy Ke <addy.ke@rock-chips.com> wrote:
>>> We need to take the card pointer in execute_tuning() for mmc_send_status(),
>>
>> mmc_send_status() is an mmc core function, not intended for host's to call.
>>
>>> but mmc->card is NULL in tuning state. So we need change the first parameter
>>> of execute_tuning() to card pointer(struct mmc_card * card).
>>
>> So, why do we need this?
>
> I asked Addy to post upstream against mmc_send_tuning(), but I guess
> he didn't (he posted against Alex's NAKed patch instead).
>
> ...when I talked to him about it, Addy was asserting that when tuning
> fails it is important (at least on dw_mmc on rk3288) that we wait for
> the card to stop being busy and that the way to detect was using
> mmc_send_status().

So, could that be due to the internal logic of the error handling in
dw_mmc driver? Or you think this is a generic issue?

According to the specifications (eMMC and SD) both states that the
tuning command has an R1 response. So, there shouldn't be any busy
signalling involved - at least according to spec.

>
> That would mean that against upstream you'd need to change
> mmc_send_tuning() to take in the card as well (or move the "host->card
> = card" assignment to before UHS init, which seems less desirable?)
>
> What do you think about that?  Is there a better solution?

Why do we need to change mmc_send_tuning()? I thought the issue was
that mmc_send_status(), which currently takes "card" as a parameter.

Kind regards
Uffe

[PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Ulf Hansson]

On 26 January 2015 at 18:45, Doug Anderson <dianders@chromium.org> wrote:
> Ulf,
>
> On Mon, Jan 26, 2015 at 7:15 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 26 January 2015 at 12:19, Addy Ke <addy.ke@rock-chips.com> wrote:
>>> We need to take the card pointer in execute_tuning() for mmc_send_status(),
>>
>> mmc_send_status() is an mmc core function, not intended for host's to call.
>>
>>> but mmc->card is NULL in tuning state. So we need change the first parameter
>>> of execute_tuning() to card pointer(struct mmc_card * card).
>>
>> So, why do we need this?
>
> I asked Addy to post upstream against mmc_send_tuning(), but I guess
> he didn't (he posted against Alex's NAKed patch instead).
>
> ...when I talked to him about it, Addy was asserting that when tuning
> fails it is important (at least on dw_mmc on rk3288) that we wait for
> the card to stop being busy and that the way to detect was using
> mmc_send_status().

So, could that be due to the internal logic of the error handling in
dw_mmc driver? Or you think this is a generic issue?

According to the specifications (eMMC and SD) both states that the
tuning command has an R1 response. So, there shouldn't be any busy
signalling involved - at least according to spec.

>
> That would mean that against upstream you'd need to change
> mmc_send_tuning() to take in the card as well (or move the "host->card
> = card" assignment to before UHS init, which seems less desirable?)
>
> What do you think about that?  Is there a better solution?

Why do we need to change mmc_send_tuning()? I thought the issue was
that mmc_send_status(), which currently takes "card" as a parameter.

Kind regards
Uffe

Re: [PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Doug Anderson]

Ulf,

On Tue, Jan 27, 2015 at 7:18 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> I asked Addy to post upstream against mmc_send_tuning(), but I guess
>> he didn't (he posted against Alex's NAKed patch instead).
>>
>> ...when I talked to him about it, Addy was asserting that when tuning
>> fails it is important (at least on dw_mmc on rk3288) that we wait for
>> the card to stop being busy and that the way to detect was using
>> mmc_send_status().
>
> So, could that be due to the internal logic of the error handling in
> dw_mmc driver? Or you think this is a generic issue?
>
> According to the specifications (eMMC and SD) both states that the
> tuning command has an R1 response. So, there shouldn't be any busy
> signalling involved - at least according to spec.

I did a bit of digging into this issue myself.  What I found was that
a "response CRC" and "end of transfer".  This was why I posted up
<https://patchwork.kernel.org/patch/5623071/>.  From that patch:

> Specifically it looks like in certain error conditions (I saw this
> with Response CRC errors) that data keeps showing up in the FIFO even
> after the error is reported and the CD (command done) bit is set.  If
> we don't wait for this data to finish transferring then it confuses
> the next transaction.  In the specific failure case I ran into I found
> that I could monitor the data_state_mc_busy bit and wait for it to
> clear, but in other failure cases this bit was stuck at busy when we
> saw an error.  Hence a generic big delay seems like the only option.

...Addy instead fixed the problem using mmc_send_status() to try to
detect when the transfer was all done and it apparently worked, but it
seemed odd to me.  My MMC "expertise" pretty much ends with looking
for simple logic errors in the MMC driver, so my hope was that one of
you guys would know this better...


>> That would mean that against upstream you'd need to change
>> mmc_send_tuning() to take in the card as well (or move the "host->card
>> = card" assignment to before UHS init, which seems less desirable?)
>>
>> What do you think about that?  Is there a better solution?
>
> Why do we need to change mmc_send_tuning()? I thought the issue was
> that mmc_send_status(), which currently takes "card" as a parameter.

Well, if mmc_send_tuning() needed to call mmc_send_status() then
mmc_send_tuning() would need the card parameter, right?


Doug

Re: [PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Doug Anderson]

Ulf,

On Tue, Jan 27, 2015 at 7:18 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> I asked Addy to post upstream against mmc_send_tuning(), but I guess
>> he didn't (he posted against Alex's NAKed patch instead).
>>
>> ...when I talked to him about it, Addy was asserting that when tuning
>> fails it is important (at least on dw_mmc on rk3288) that we wait for
>> the card to stop being busy and that the way to detect was using
>> mmc_send_status().
>
> So, could that be due to the internal logic of the error handling in
> dw_mmc driver? Or you think this is a generic issue?
>
> According to the specifications (eMMC and SD) both states that the
> tuning command has an R1 response. So, there shouldn't be any busy
> signalling involved - at least according to spec.

I did a bit of digging into this issue myself.  What I found was that
a "response CRC" and "end of transfer".  This was why I posted up
<https://patchwork.kernel.org/patch/5623071/>.  From that patch:

> Specifically it looks like in certain error conditions (I saw this
> with Response CRC errors) that data keeps showing up in the FIFO even
> after the error is reported and the CD (command done) bit is set.  If
> we don't wait for this data to finish transferring then it confuses
> the next transaction.  In the specific failure case I ran into I found
> that I could monitor the data_state_mc_busy bit and wait for it to
> clear, but in other failure cases this bit was stuck at busy when we
> saw an error.  Hence a generic big delay seems like the only option.

...Addy instead fixed the problem using mmc_send_status() to try to
detect when the transfer was all done and it apparently worked, but it
seemed odd to me.  My MMC "expertise" pretty much ends with looking
for simple logic errors in the MMC driver, so my hope was that one of
you guys would know this better...


>> That would mean that against upstream you'd need to change
>> mmc_send_tuning() to take in the card as well (or move the "host->card
>> = card" assignment to before UHS init, which seems less desirable?)
>>
>> What do you think about that?  Is there a better solution?
>
> Why do we need to change mmc_send_tuning()? I thought the issue was
> that mmc_send_status(), which currently takes "card" as a parameter.

Well, if mmc_send_tuning() needed to call mmc_send_status() then
mmc_send_tuning() would need the card parameter, right?


Doug

[PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Doug Anderson]

Ulf,

On Tue, Jan 27, 2015 at 7:18 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> I asked Addy to post upstream against mmc_send_tuning(), but I guess
>> he didn't (he posted against Alex's NAKed patch instead).
>>
>> ...when I talked to him about it, Addy was asserting that when tuning
>> fails it is important (at least on dw_mmc on rk3288) that we wait for
>> the card to stop being busy and that the way to detect was using
>> mmc_send_status().
>
> So, could that be due to the internal logic of the error handling in
> dw_mmc driver? Or you think this is a generic issue?
>
> According to the specifications (eMMC and SD) both states that the
> tuning command has an R1 response. So, there shouldn't be any busy
> signalling involved - at least according to spec.

I did a bit of digging into this issue myself.  What I found was that
a "response CRC" and "end of transfer".  This was why I posted up
<https://patchwork.kernel.org/patch/5623071/>.  From that patch:

> Specifically it looks like in certain error conditions (I saw this
> with Response CRC errors) that data keeps showing up in the FIFO even
> after the error is reported and the CD (command done) bit is set.  If
> we don't wait for this data to finish transferring then it confuses
> the next transaction.  In the specific failure case I ran into I found
> that I could monitor the data_state_mc_busy bit and wait for it to
> clear, but in other failure cases this bit was stuck at busy when we
> saw an error.  Hence a generic big delay seems like the only option.

...Addy instead fixed the problem using mmc_send_status() to try to
detect when the transfer was all done and it apparently worked, but it
seemed odd to me.  My MMC "expertise" pretty much ends with looking
for simple logic errors in the MMC driver, so my hope was that one of
you guys would know this better...


>> That would mean that against upstream you'd need to change
>> mmc_send_tuning() to take in the card as well (or move the "host->card
>> = card" assignment to before UHS init, which seems less desirable?)
>>
>> What do you think about that?  Is there a better solution?
>
> Why do we need to change mmc_send_tuning()? I thought the issue was
> that mmc_send_status(), which currently takes "card" as a parameter.

Well, if mmc_send_tuning() needed to call mmc_send_status() then
mmc_send_tuning() would need the card parameter, right?


Doug

Re: [PATCH 2/2] mmc: dw_mmc: wait until card ready if tuning fails

[Ulf Hansson]

On 26 January 2015 at 12:19, Addy Ke <addy.ke@rock-chips.com> wrote:
> This patch based on Alex's patch:
> https://patchwork.kernel.org/patch/5516411/

This above patch was rejected, since it doesn't use mmc_send_tuning().
Please base you work on my latest next branch.

If you need other patches which has been posted recently, which hasn’t
been reviewed nor merged you can just included them in your patchset
as well.

Kind regards
Uffe

>
> Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
> ---
>  drivers/mmc/core/mmc_ops.h |  1 -
>  drivers/mmc/host/dw_mmc.c  | 48 ++++++++++++++++++++++++++++++++++++++++------
>  include/linux/mmc/card.h   |  2 ++
>  3 files changed, 44 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
> index 6f4b00e..c5be9ce 100644
> --- a/drivers/mmc/core/mmc_ops.h
> +++ b/drivers/mmc/core/mmc_ops.h
> @@ -20,7 +20,6 @@ int mmc_send_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr);
>  int mmc_all_send_cid(struct mmc_host *host, u32 *cid);
>  int mmc_set_relative_addr(struct mmc_card *card);
>  int mmc_send_csd(struct mmc_card *card, u32 *csd);
> -int mmc_send_status(struct mmc_card *card, u32 *status);
>  int mmc_send_cid(struct mmc_host *host, u32 *cid);
>  int mmc_spi_read_ocr(struct mmc_host *host, int highcap, u32 *ocrp);
>  int mmc_spi_set_crc(struct mmc_host *host, int use_crc);
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index e54e656..4a31a5e 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1317,10 +1317,38 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
>         spin_unlock_irqrestore(&host->irq_lock, irqflags);
>  }
>
> -static int dw_mci_tuning_test(struct dw_mci_slot *slot, u32 opcode,
> -                              struct dw_mci_tuning_data *tuning_data,
> -                              u8 *blk_test)
> +static int dw_mci_card_busy(u32 status)
>  {
> +       return !(status & R1_READY_FOR_DATA) ||
> +               (R1_CURRENT_STATE(status) == R1_STATE_PRG);
> +}
> +
> +static int dw_mci_wait_for_card_ready(struct mmc_card *card)
> +{
> +       struct mmc_host *mmc = card->host;
> +       struct dw_mci_slot *slot = mmc_priv(mmc);
> +       struct dw_mci *host = slot->host;
> +       int err = 0;
> +       u32 status;
> +
> +       do {
> +               err = mmc_send_status(card, &status);
> +               if (err) {
> +                       dev_err(host->dev,
> +                               "Get card status fail in tuning state\n");
> +                       break;
> +               }
> +       } while (dw_mci_card_busy(status));
> +
> +       return err;
> +}
> +
> +static int dw_mci_tuning_test(struct mmc_card *card, u32 opcode,
> +                             struct dw_mci_tuning_data *tuning_data,
> +                             u8 *blk_test)
> +{
> +       struct mmc_host *mmc = card->host;
> +       struct dw_mci_slot *slot = mmc_priv(mmc);
>         struct dw_mci *host = slot->host;
>         struct mmc_host *mmc = slot->mmc;
>         const u8 *blk_pattern = tuning_data->blk_pattern;
> @@ -1330,6 +1358,7 @@ static int dw_mci_tuning_test(struct dw_mci_slot *slot, u32 opcode,
>         struct mmc_command stop = {0};
>         struct mmc_data data = {0};
>         struct scatterlist sg;
> +       int err;
>
>         memset(blk_test, 0, blksz);
>
> @@ -1365,6 +1394,11 @@ static int dw_mci_tuning_test(struct dw_mci_slot *slot, u32 opcode,
>                 dev_dbg(host->dev,
>                         "Tuning error: cmd.error:%d, data.error:%d\n",
>                         cmd.error, data.error);
> +
> +               err = dw_mci_wait_for_card_ready(card);
> +               if (err)
> +                       return err;
> +
>                 if (cmd.error)
>                         return cmd.error;
>                 else
> @@ -1372,9 +1406,11 @@ static int dw_mci_tuning_test(struct dw_mci_slot *slot, u32 opcode,
>         }
>  }
>
> -static int dw_mci_execute_generic_tuning(struct dw_mci_slot *slot, u32 opcode,
> +static int dw_mci_execute_generic_tuning(struct mmc_card *card, u32 opcode,
>                                          struct dw_mci_tuning_data *tuning_data)
>  {
> +       struct mmc_host *mmc = card->host;
> +       struct dw_mci_slot *slot = mmc_priv(mmc);
>         struct dw_mci *host = slot->host;
>         unsigned int blksz = tuning_data->blksz;
>         u8 *blk_test;
> @@ -1412,7 +1448,7 @@ static int dw_mci_execute_generic_tuning(struct dw_mci_slot *slot, u32 opcode,
>         for (i = 0; i < NUM_PHASES; i++) {
>                 clk_set_phase(host->sample_clk, i * PHASE_INCREMENT);
>
> -               v = !dw_mci_tuning_test(slot, opcode, tuning_data, blk_test);
> +               v = !dw_mci_tuning_test(card, opcode, tuning_data, blk_test);
>
>                 if ((!prev_v) && v) {
>                         range_count++;
> @@ -1496,7 +1532,7 @@ static int dw_mci_execute_tuning(struct mmc_card *card, u32 opcode)
>         if (drv_data && drv_data->execute_tuning)
>                 err = drv_data->execute_tuning(slot, opcode, &tuning_data);
>         else
> -               err = dw_mci_execute_generic_tuning(slot, opcode, &tuning_data);
> +               err = dw_mci_execute_generic_tuning(card, opcode, &tuning_data);
>         return err;
>  }
>
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index 4d69c00..40d90ae 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -518,4 +518,6 @@ extern void mmc_unregister_driver(struct device_driver *);
>  extern void mmc_fixup_device(struct mmc_card *card,
>                              const struct mmc_fixup *table);
>
> +int mmc_send_status(struct mmc_card *card, u32 *status);
> +
>  #endif /* LINUX_MMC_CARD_H */
> --
> 1.8.3.2
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] mmc: dw_mmc: wait until card ready if tuning fails

[Ulf Hansson]

On 26 January 2015 at 12:19, Addy Ke <addy.ke@rock-chips.com> wrote:
> This patch based on Alex's patch:
> https://patchwork.kernel.org/patch/5516411/

This above patch was rejected, since it doesn't use mmc_send_tuning().
Please base you work on my latest next branch.

If you need other patches which has been posted recently, which hasn’t
been reviewed nor merged you can just included them in your patchset
as well.

Kind regards
Uffe

>
> Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
> ---
>  drivers/mmc/core/mmc_ops.h |  1 -
>  drivers/mmc/host/dw_mmc.c  | 48 ++++++++++++++++++++++++++++++++++++++++------
>  include/linux/mmc/card.h   |  2 ++
>  3 files changed, 44 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
> index 6f4b00e..c5be9ce 100644
> --- a/drivers/mmc/core/mmc_ops.h
> +++ b/drivers/mmc/core/mmc_ops.h
> @@ -20,7 +20,6 @@ int mmc_send_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr);
>  int mmc_all_send_cid(struct mmc_host *host, u32 *cid);
>  int mmc_set_relative_addr(struct mmc_card *card);
>  int mmc_send_csd(struct mmc_card *card, u32 *csd);
> -int mmc_send_status(struct mmc_card *card, u32 *status);
>  int mmc_send_cid(struct mmc_host *host, u32 *cid);
>  int mmc_spi_read_ocr(struct mmc_host *host, int highcap, u32 *ocrp);
>  int mmc_spi_set_crc(struct mmc_host *host, int use_crc);
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index e54e656..4a31a5e 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1317,10 +1317,38 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
>         spin_unlock_irqrestore(&host->irq_lock, irqflags);
>  }
>
> -static int dw_mci_tuning_test(struct dw_mci_slot *slot, u32 opcode,
> -                              struct dw_mci_tuning_data *tuning_data,
> -                              u8 *blk_test)
> +static int dw_mci_card_busy(u32 status)
>  {
> +       return !(status & R1_READY_FOR_DATA) ||
> +               (R1_CURRENT_STATE(status) == R1_STATE_PRG);
> +}
> +
> +static int dw_mci_wait_for_card_ready(struct mmc_card *card)
> +{
> +       struct mmc_host *mmc = card->host;
> +       struct dw_mci_slot *slot = mmc_priv(mmc);
> +       struct dw_mci *host = slot->host;
> +       int err = 0;
> +       u32 status;
> +
> +       do {
> +               err = mmc_send_status(card, &status);
> +               if (err) {
> +                       dev_err(host->dev,
> +                               "Get card status fail in tuning state\n");
> +                       break;
> +               }
> +       } while (dw_mci_card_busy(status));
> +
> +       return err;
> +}
> +
> +static int dw_mci_tuning_test(struct mmc_card *card, u32 opcode,
> +                             struct dw_mci_tuning_data *tuning_data,
> +                             u8 *blk_test)
> +{
> +       struct mmc_host *mmc = card->host;
> +       struct dw_mci_slot *slot = mmc_priv(mmc);
>         struct dw_mci *host = slot->host;
>         struct mmc_host *mmc = slot->mmc;
>         const u8 *blk_pattern = tuning_data->blk_pattern;
> @@ -1330,6 +1358,7 @@ static int dw_mci_tuning_test(struct dw_mci_slot *slot, u32 opcode,
>         struct mmc_command stop = {0};
>         struct mmc_data data = {0};
>         struct scatterlist sg;
> +       int err;
>
>         memset(blk_test, 0, blksz);
>
> @@ -1365,6 +1394,11 @@ static int dw_mci_tuning_test(struct dw_mci_slot *slot, u32 opcode,
>                 dev_dbg(host->dev,
>                         "Tuning error: cmd.error:%d, data.error:%d\n",
>                         cmd.error, data.error);
> +
> +               err = dw_mci_wait_for_card_ready(card);
> +               if (err)
> +                       return err;
> +
>                 if (cmd.error)
>                         return cmd.error;
>                 else
> @@ -1372,9 +1406,11 @@ static int dw_mci_tuning_test(struct dw_mci_slot *slot, u32 opcode,
>         }
>  }
>
> -static int dw_mci_execute_generic_tuning(struct dw_mci_slot *slot, u32 opcode,
> +static int dw_mci_execute_generic_tuning(struct mmc_card *card, u32 opcode,
>                                          struct dw_mci_tuning_data *tuning_data)
>  {
> +       struct mmc_host *mmc = card->host;
> +       struct dw_mci_slot *slot = mmc_priv(mmc);
>         struct dw_mci *host = slot->host;
>         unsigned int blksz = tuning_data->blksz;
>         u8 *blk_test;
> @@ -1412,7 +1448,7 @@ static int dw_mci_execute_generic_tuning(struct dw_mci_slot *slot, u32 opcode,
>         for (i = 0; i < NUM_PHASES; i++) {
>                 clk_set_phase(host->sample_clk, i * PHASE_INCREMENT);
>
> -               v = !dw_mci_tuning_test(slot, opcode, tuning_data, blk_test);
> +               v = !dw_mci_tuning_test(card, opcode, tuning_data, blk_test);
>
>                 if ((!prev_v) && v) {
>                         range_count++;
> @@ -1496,7 +1532,7 @@ static int dw_mci_execute_tuning(struct mmc_card *card, u32 opcode)
>         if (drv_data && drv_data->execute_tuning)
>                 err = drv_data->execute_tuning(slot, opcode, &tuning_data);
>         else
> -               err = dw_mci_execute_generic_tuning(slot, opcode, &tuning_data);
> +               err = dw_mci_execute_generic_tuning(card, opcode, &tuning_data);
>         return err;
>  }
>
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index 4d69c00..40d90ae 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -518,4 +518,6 @@ extern void mmc_unregister_driver(struct device_driver *);
>  extern void mmc_fixup_device(struct mmc_card *card,
>                              const struct mmc_fixup *table);
>
> +int mmc_send_status(struct mmc_card *card, u32 *status);
> +
>  #endif /* LINUX_MMC_CARD_H */
> --
> 1.8.3.2
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/2] mmc: dw_mmc: wait until card ready if tuning fails

[Ulf Hansson]

On 26 January 2015 at 12:19, Addy Ke <addy.ke@rock-chips.com> wrote:
> This patch based on Alex's patch:
> https://patchwork.kernel.org/patch/5516411/

This above patch was rejected, since it doesn't use mmc_send_tuning().
Please base you work on my latest next branch.

If you need other patches which has been posted recently, which hasn?t
been reviewed nor merged you can just included them in your patchset
as well.

Kind regards
Uffe

>
> Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
> ---
>  drivers/mmc/core/mmc_ops.h |  1 -
>  drivers/mmc/host/dw_mmc.c  | 48 ++++++++++++++++++++++++++++++++++++++++------
>  include/linux/mmc/card.h   |  2 ++
>  3 files changed, 44 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
> index 6f4b00e..c5be9ce 100644
> --- a/drivers/mmc/core/mmc_ops.h
> +++ b/drivers/mmc/core/mmc_ops.h
> @@ -20,7 +20,6 @@ int mmc_send_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr);
>  int mmc_all_send_cid(struct mmc_host *host, u32 *cid);
>  int mmc_set_relative_addr(struct mmc_card *card);
>  int mmc_send_csd(struct mmc_card *card, u32 *csd);
> -int mmc_send_status(struct mmc_card *card, u32 *status);
>  int mmc_send_cid(struct mmc_host *host, u32 *cid);
>  int mmc_spi_read_ocr(struct mmc_host *host, int highcap, u32 *ocrp);
>  int mmc_spi_set_crc(struct mmc_host *host, int use_crc);
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index e54e656..4a31a5e 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1317,10 +1317,38 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
>         spin_unlock_irqrestore(&host->irq_lock, irqflags);
>  }
>
> -static int dw_mci_tuning_test(struct dw_mci_slot *slot, u32 opcode,
> -                              struct dw_mci_tuning_data *tuning_data,
> -                              u8 *blk_test)
> +static int dw_mci_card_busy(u32 status)
>  {
> +       return !(status & R1_READY_FOR_DATA) ||
> +               (R1_CURRENT_STATE(status) == R1_STATE_PRG);
> +}
> +
> +static int dw_mci_wait_for_card_ready(struct mmc_card *card)
> +{
> +       struct mmc_host *mmc = card->host;
> +       struct dw_mci_slot *slot = mmc_priv(mmc);
> +       struct dw_mci *host = slot->host;
> +       int err = 0;
> +       u32 status;
> +
> +       do {
> +               err = mmc_send_status(card, &status);
> +               if (err) {
> +                       dev_err(host->dev,
> +                               "Get card status fail in tuning state\n");
> +                       break;
> +               }
> +       } while (dw_mci_card_busy(status));
> +
> +       return err;
> +}
> +
> +static int dw_mci_tuning_test(struct mmc_card *card, u32 opcode,
> +                             struct dw_mci_tuning_data *tuning_data,
> +                             u8 *blk_test)
> +{
> +       struct mmc_host *mmc = card->host;
> +       struct dw_mci_slot *slot = mmc_priv(mmc);
>         struct dw_mci *host = slot->host;
>         struct mmc_host *mmc = slot->mmc;
>         const u8 *blk_pattern = tuning_data->blk_pattern;
> @@ -1330,6 +1358,7 @@ static int dw_mci_tuning_test(struct dw_mci_slot *slot, u32 opcode,
>         struct mmc_command stop = {0};
>         struct mmc_data data = {0};
>         struct scatterlist sg;
> +       int err;
>
>         memset(blk_test, 0, blksz);
>
> @@ -1365,6 +1394,11 @@ static int dw_mci_tuning_test(struct dw_mci_slot *slot, u32 opcode,
>                 dev_dbg(host->dev,
>                         "Tuning error: cmd.error:%d, data.error:%d\n",
>                         cmd.error, data.error);
> +
> +               err = dw_mci_wait_for_card_ready(card);
> +               if (err)
> +                       return err;
> +
>                 if (cmd.error)
>                         return cmd.error;
>                 else
> @@ -1372,9 +1406,11 @@ static int dw_mci_tuning_test(struct dw_mci_slot *slot, u32 opcode,
>         }
>  }
>
> -static int dw_mci_execute_generic_tuning(struct dw_mci_slot *slot, u32 opcode,
> +static int dw_mci_execute_generic_tuning(struct mmc_card *card, u32 opcode,
>                                          struct dw_mci_tuning_data *tuning_data)
>  {
> +       struct mmc_host *mmc = card->host;
> +       struct dw_mci_slot *slot = mmc_priv(mmc);
>         struct dw_mci *host = slot->host;
>         unsigned int blksz = tuning_data->blksz;
>         u8 *blk_test;
> @@ -1412,7 +1448,7 @@ static int dw_mci_execute_generic_tuning(struct dw_mci_slot *slot, u32 opcode,
>         for (i = 0; i < NUM_PHASES; i++) {
>                 clk_set_phase(host->sample_clk, i * PHASE_INCREMENT);
>
> -               v = !dw_mci_tuning_test(slot, opcode, tuning_data, blk_test);
> +               v = !dw_mci_tuning_test(card, opcode, tuning_data, blk_test);
>
>                 if ((!prev_v) && v) {
>                         range_count++;
> @@ -1496,7 +1532,7 @@ static int dw_mci_execute_tuning(struct mmc_card *card, u32 opcode)
>         if (drv_data && drv_data->execute_tuning)
>                 err = drv_data->execute_tuning(slot, opcode, &tuning_data);
>         else
> -               err = dw_mci_execute_generic_tuning(slot, opcode, &tuning_data);
> +               err = dw_mci_execute_generic_tuning(card, opcode, &tuning_data);
>         return err;
>  }
>
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index 4d69c00..40d90ae 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -518,4 +518,6 @@ extern void mmc_unregister_driver(struct device_driver *);
>  extern void mmc_fixup_device(struct mmc_card *card,
>                              const struct mmc_fixup *table);
>
> +int mmc_send_status(struct mmc_card *card, u32 *status);
> +
>  #endif /* LINUX_MMC_CARD_H */
> --
> 1.8.3.2
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Ulf Hansson]

- Drastically decreased cc-list.

On 29 January 2015 at 01:55, Doug Anderson <dianders@chromium.org> wrote:
> Ulf,
>
> On Tue, Jan 27, 2015 at 7:18 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> I asked Addy to post upstream against mmc_send_tuning(), but I guess
>>> he didn't (he posted against Alex's NAKed patch instead).
>>>
>>> ...when I talked to him about it, Addy was asserting that when tuning
>>> fails it is important (at least on dw_mmc on rk3288) that we wait for
>>> the card to stop being busy and that the way to detect was using
>>> mmc_send_status().
>>
>> So, could that be due to the internal logic of the error handling in
>> dw_mmc driver? Or you think this is a generic issue?
>>
>> According to the specifications (eMMC and SD) both states that the
>> tuning command has an R1 response. So, there shouldn't be any busy
>> signalling involved - at least according to spec.
>
> I did a bit of digging into this issue myself.  What I found was that
> a "response CRC" and "end of transfer".  This was why I posted up
> <https://patchwork.kernel.org/patch/5623071/>.  From that patch:
>
>> Specifically it looks like in certain error conditions (I saw this
>> with Response CRC errors) that data keeps showing up in the FIFO even
>> after the error is reported and the CD (command done) bit is set.  If
>> we don't wait for this data to finish transferring then it confuses
>> the next transaction.  In the specific failure case I ran into I found
>> that I could monitor the data_state_mc_busy bit and wait for it to
>> clear, but in other failure cases this bit was stuck at busy when we
>> saw an error.  Hence a generic big delay seems like the only option.

I haven't queued that patch, I was waiting for an ack from Seungwon or Jaehoon.

Do you think it could solve this issue, we could give it a try!?

>
> ...Addy instead fixed the problem using mmc_send_status() to try to
> detect when the transfer was all done and it apparently worked, but it
> seemed odd to me.  My MMC "expertise" pretty much ends with looking
> for simple logic errors in the MMC driver, so my hope was that one of
> you guys would know this better...
>
>
>>> That would mean that against upstream you'd need to change
>>> mmc_send_tuning() to take in the card as well (or move the "host->card
>>> = card" assignment to before UHS init, which seems less desirable?)

I get your point now.

Changing mmc_send_tuning() to take "card" will work due to $subject
patch changed the ->execute_tuning() callbacks to take "card" as well.

>>>
>>> What do you think about that?  Is there a better solution?
>>
>> Why do we need to change mmc_send_tuning()? I thought the issue was
>> that mmc_send_status(), which currently takes "card" as a parameter.
>
> Well, if mmc_send_tuning() needed to call mmc_send_status() then
> mmc_send_tuning() would need the card parameter, right?

Correct, got it now. :-)

I didn't understand that you wanted mmc_send_tuning() to invoke
mmc_send_status() while it got some errors. From Addy's patch2,
mmc_send_status() is invoked from the host driver.

Anyway, I think we should follow your suggestion to change the
behaviour of mmc_send_tuning(). Though, I think it should use
bus_ops->alive() callback instead (and that callback then also need to
change to take "card" as a parameter), since that would be generic and
the cover the SDIO case as well.

Kind regards
Uffe

Re: [PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Doug Anderson]

Ulf,

On Thu, Jan 29, 2015 at 1:16 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> - Drastically decreased cc-list.
>
> On 29 January 2015 at 01:55, Doug Anderson <dianders@chromium.org> wrote:
>> Ulf,
>>
>> On Tue, Jan 27, 2015 at 7:18 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> I asked Addy to post upstream against mmc_send_tuning(), but I guess
>>>> he didn't (he posted against Alex's NAKed patch instead).
>>>>
>>>> ...when I talked to him about it, Addy was asserting that when tuning
>>>> fails it is important (at least on dw_mmc on rk3288) that we wait for
>>>> the card to stop being busy and that the way to detect was using
>>>> mmc_send_status().
>>>
>>> So, could that be due to the internal logic of the error handling in
>>> dw_mmc driver? Or you think this is a generic issue?
>>>
>>> According to the specifications (eMMC and SD) both states that the
>>> tuning command has an R1 response. So, there shouldn't be any busy
>>> signalling involved - at least according to spec.
>>
>> I did a bit of digging into this issue myself.  What I found was that
>> a "response CRC" and "end of transfer".  This was why I posted up
>> <https://patchwork.kernel.org/patch/5623071/>.  From that patch:
>>
>>> Specifically it looks like in certain error conditions (I saw this
>>> with Response CRC errors) that data keeps showing up in the FIFO even
>>> after the error is reported and the CD (command done) bit is set.  If
>>> we don't wait for this data to finish transferring then it confuses
>>> the next transaction.  In the specific failure case I ran into I found
>>> that I could monitor the data_state_mc_busy bit and wait for it to
>>> clear, but in other failure cases this bit was stuck at busy when we
>>> saw an error.  Hence a generic big delay seems like the only option.
>
> I haven't queued that patch, I was waiting for an ack from Seungwon or Jaehoon.
>
> Do you think it could solve this issue, we could give it a try!?

My big fat delay does seem to solve the issue, but it has the side
effect of slowing down tuning quite a bit so I'd rather find a more
proper fix.  We're talking several hundred extra milliseconds slower
per slot that is tuned...

I still don't exactly have a warm fuzzy about using the send_status()
command like this, but it seems to work (actually, I should verify
that myself--I've been taking Addy's word that his solution works).  I
do wish someone could tell me "oh right, yeah, we do need a
send_status in that case".  ;)  Addy said that in the non-tuning case
that the core will always do a send_status so that this fix is really
only for tuning and doesn't need to be applied in general.  I also
haven't validated that myself...

Overall it does sorta seem like this might just be a quirk with the
rk3288 dw_mmc.  It feels like the controller is in a wonky state and
maybe this extra send_status helps it get out?


>> ...Addy instead fixed the problem using mmc_send_status() to try to
>> detect when the transfer was all done and it apparently worked, but it
>> seemed odd to me.  My MMC "expertise" pretty much ends with looking
>> for simple logic errors in the MMC driver, so my hope was that one of
>> you guys would know this better...
>>
>>
>>>> That would mean that against upstream you'd need to change
>>>> mmc_send_tuning() to take in the card as well (or move the "host->card
>>>> = card" assignment to before UHS init, which seems less desirable?)
>
> I get your point now.
>
> Changing mmc_send_tuning() to take "card" will work due to $subject
> patch changed the ->execute_tuning() callbacks to take "card" as well.
>
>>>>
>>>> What do you think about that?  Is there a better solution?
>>>
>>> Why do we need to change mmc_send_tuning()? I thought the issue was
>>> that mmc_send_status(), which currently takes "card" as a parameter.
>>
>> Well, if mmc_send_tuning() needed to call mmc_send_status() then
>> mmc_send_tuning() would need the card parameter, right?
>
> Correct, got it now. :-)
>
> I didn't understand that you wanted mmc_send_tuning() to invoke
> mmc_send_status() while it got some errors. From Addy's patch2,
> mmc_send_status() is invoked from the host driver.
>
> Anyway, I think we should follow your suggestion to change the
> behaviour of mmc_send_tuning(). Though, I think it should use
> bus_ops->alive() callback instead (and that callback then also need to
> change to take "card" as a parameter), since that would be generic and
> the cover the SDIO case as well.

That sounds reasonable to me.

Addy: you've been very quiet.  What do you think?

-Doug

Re: [PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[addy ke]

hi, Doug

On 2015/1/30 08:13, Doug Anderson wrote:
> Ulf,
> 
> On Thu, Jan 29, 2015 at 1:16 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> - Drastically decreased cc-list.
>>
>> On 29 January 2015 at 01:55, Doug Anderson <dianders@chromium.org> wrote:
>>> Ulf,
>>>
>>> On Tue, Jan 27, 2015 at 7:18 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>> I asked Addy to post upstream against mmc_send_tuning(), but I guess
>>>>> he didn't (he posted against Alex's NAKed patch instead).
>>>>>
>>>>> ...when I talked to him about it, Addy was asserting that when tuning
>>>>> fails it is important (at least on dw_mmc on rk3288) that we wait for
>>>>> the card to stop being busy and that the way to detect was using
>>>>> mmc_send_status().
>>>>
>>>> So, could that be due to the internal logic of the error handling in
>>>> dw_mmc driver? Or you think this is a generic issue?
>>>>
>>>> According to the specifications (eMMC and SD) both states that the
>>>> tuning command has an R1 response. So, there shouldn't be any busy
>>>> signalling involved - at least according to spec.
>>>
>>> I did a bit of digging into this issue myself.  What I found was that
>>> a "response CRC" and "end of transfer".  This was why I posted up
>>> <https://patchwork.kernel.org/patch/5623071/>.  From that patch:
>>>
>>>> Specifically it looks like in certain error conditions (I saw this
>>>> with Response CRC errors) that data keeps showing up in the FIFO even
>>>> after the error is reported and the CD (command done) bit is set.  If
>>>> we don't wait for this data to finish transferring then it confuses
>>>> the next transaction.  In the specific failure case I ran into I found
>>>> that I could monitor the data_state_mc_busy bit and wait for it to
>>>> clear, but in other failure cases this bit was stuck at busy when we
>>>> saw an error.  Hence a generic big delay seems like the only option.
>>
>> I haven't queued that patch, I was waiting for an ack from Seungwon or Jaehoon.
>>
>> Do you think it could solve this issue, we could give it a try!?
> 
> My big fat delay does seem to solve the issue, but it has the side
> effect of slowing down tuning quite a bit so I'd rather find a more
> proper fix.  We're talking several hundred extra milliseconds slower
> per slot that is tuned...
> 
> I still don't exactly have a warm fuzzy about using the send_status()
> command like this, but it seems to work (actually, I should verify
> that myself--I've been taking Addy's word that his solution works).  I
> do wish someone could tell me "oh right, yeah, we do need a
> send_status in that case".  ;)  Addy said that in the non-tuning case
> that the core will always do a send_status so that this fix is really
> only for tuning and doesn't need to be applied in general.  I also
> haven't validated that myself...
> 
> Overall it does sorta seem like this might just be a quirk with the
> rk3288 dw_mmc.  It feels like the controller is in a wonky state and
> maybe this extra send_status helps it get out?
> 
> 
>>> ...Addy instead fixed the problem using mmc_send_status() to try to
>>> detect when the transfer was all done and it apparently worked, but it
>>> seemed odd to me.  My MMC "expertise" pretty much ends with looking
>>> for simple logic errors in the MMC driver, so my hope was that one of
>>> you guys would know this better...
>>>
>>>
>>>>> That would mean that against upstream you'd need to change
>>>>> mmc_send_tuning() to take in the card as well (or move the "host->card
>>>>> = card" assignment to before UHS init, which seems less desirable?)
>>
>> I get your point now.
>>
>> Changing mmc_send_tuning() to take "card" will work due to $subject
>> patch changed the ->execute_tuning() callbacks to take "card" as well.
>>
>>>>>
>>>>> What do you think about that?  Is there a better solution?
>>>>
>>>> Why do we need to change mmc_send_tuning()? I thought the issue was
>>>> that mmc_send_status(), which currently takes "card" as a parameter.
>>>
>>> Well, if mmc_send_tuning() needed to call mmc_send_status() then
>>> mmc_send_tuning() would need the card parameter, right?
>>
>> Correct, got it now. :-)
>>
>> I didn't understand that you wanted mmc_send_tuning() to invoke
>> mmc_send_status() while it got some errors. From Addy's patch2,
>> mmc_send_status() is invoked from the host driver.
>>
>> Anyway, I think we should follow your suggestion to change the
>> behaviour of mmc_send_tuning(). Though, I think it should use
>> bus_ops->alive() callback instead (and that callback then also need to
>> change to take "card" as a parameter), since that would be generic and
>> the cover the SDIO case as well.
> 
> That sounds reasonable to me.
> 
> Addy: you've been very quiet.  What do you think?
Sorry for reply late.
I am busy with some other important things, and can't confirm it by pink2 board.

about bus_ops->alive, I think it can't use in tuning state.
Because:
bus_ops->alive() --> mmc_sd_alive(host) /* sd card */ -->mmc_send_status(host->card, NULL);
But host->card == NULL in tuning state(mmc_sd_init_card->mmc_sd_init_uhs_card).

Only if sd is initialized successfully, we can get card pointer by host->card.
see: mmc_sd_init_card(drivers/mmc/core/sd.c), the end of this function: host->card = card
> 
> -Doug
> 
> 
> 

Re: [PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Jaehoon Chung]

On 01/30/2015 09:13 AM, Doug Anderson wrote:
> Ulf,
> 
> On Thu, Jan 29, 2015 at 1:16 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> - Drastically decreased cc-list.
>>
>> On 29 January 2015 at 01:55, Doug Anderson <dianders@chromium.org> wrote:
>>> Ulf,
>>>
>>> On Tue, Jan 27, 2015 at 7:18 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>> I asked Addy to post upstream against mmc_send_tuning(), but I guess
>>>>> he didn't (he posted against Alex's NAKed patch instead).
>>>>>
>>>>> ...when I talked to him about it, Addy was asserting that when tuning
>>>>> fails it is important (at least on dw_mmc on rk3288) that we wait for
>>>>> the card to stop being busy and that the way to detect was using
>>>>> mmc_send_status().
>>>>
>>>> So, could that be due to the internal logic of the error handling in
>>>> dw_mmc driver? Or you think this is a generic issue?
>>>>
>>>> According to the specifications (eMMC and SD) both states that the
>>>> tuning command has an R1 response. So, there shouldn't be any busy
>>>> signalling involved - at least according to spec.
>>>
>>> I did a bit of digging into this issue myself.  What I found was that
>>> a "response CRC" and "end of transfer".  This was why I posted up
>>> <https://patchwork.kernel.org/patch/5623071/>.  From that patch:
>>>
>>>> Specifically it looks like in certain error conditions (I saw this
>>>> with Response CRC errors) that data keeps showing up in the FIFO even
>>>> after the error is reported and the CD (command done) bit is set.  If
>>>> we don't wait for this data to finish transferring then it confuses
>>>> the next transaction.  In the specific failure case I ran into I found
>>>> that I could monitor the data_state_mc_busy bit and wait for it to
>>>> clear, but in other failure cases this bit was stuck at busy when we
>>>> saw an error.  Hence a generic big delay seems like the only option.
>>
>> I haven't queued that patch, I was waiting for an ack from Seungwon or Jaehoon.

Sorry, which patch? i missed it.

>>
>> Do you think it could solve this issue, we could give it a try!?
> 
> My big fat delay does seem to solve the issue, but it has the side
> effect of slowing down tuning quite a bit so I'd rather find a more
> proper fix.  We're talking several hundred extra milliseconds slower
> per slot that is tuned...
> 
> I still don't exactly have a warm fuzzy about using the send_status()
> command like this, but it seems to work (actually, I should verify
> that myself--I've been taking Addy's word that his solution works).  I
> do wish someone could tell me "oh right, yeah, we do need a
> send_status in that case".  ;)  Addy said that in the non-tuning case
> that the core will always do a send_status so that this fix is really
> only for tuning and doesn't need to be applied in general.  I also
> haven't validated that myself...
> 
> Overall it does sorta seem like this might just be a quirk with the
> rk3288 dw_mmc.  It feels like the controller is in a wonky state and
> maybe this extra send_status helps it get out?

I think that dw-mmc controller seems to have the main problem for tuning sequence.
In my knowledge, tuning sequence doesn't need to send the stop command.
but the code of dw-mmc need to handle the stop command and it has implemented.
Using Send_status() is good solution, but i think it's not main solution.

I will work to clean up the code for dwmmc controller.
And consider to handle the stop command..

Best Regards,
Jaehoon Chung
> 
> 
>>> ...Addy instead fixed the problem using mmc_send_status() to try to
>>> detect when the transfer was all done and it apparently worked, but it
>>> seemed odd to me.  My MMC "expertise" pretty much ends with looking
>>> for simple logic errors in the MMC driver, so my hope was that one of
>>> you guys would know this better...
>>>
>>>
>>>>> That would mean that against upstream you'd need to change
>>>>> mmc_send_tuning() to take in the card as well (or move the "host->card
>>>>> = card" assignment to before UHS init, which seems less desirable?)
>>
>> I get your point now.
>>
>> Changing mmc_send_tuning() to take "card" will work due to $subject
>> patch changed the ->execute_tuning() callbacks to take "card" as well.
>>
>>>>>
>>>>> What do you think about that?  Is there a better solution?
>>>>
>>>> Why do we need to change mmc_send_tuning()? I thought the issue was
>>>> that mmc_send_status(), which currently takes "card" as a parameter.
>>>
>>> Well, if mmc_send_tuning() needed to call mmc_send_status() then
>>> mmc_send_tuning() would need the card parameter, right?
>>
>> Correct, got it now. :-)
>>
>> I didn't understand that you wanted mmc_send_tuning() to invoke
>> mmc_send_status() while it got some errors. From Addy's patch2,
>> mmc_send_status() is invoked from the host driver.
>>
>> Anyway, I think we should follow your suggestion to change the
>> behaviour of mmc_send_tuning(). Though, I think it should use
>> bus_ops->alive() callback instead (and that callback then also need to
>> change to take "card" as a parameter), since that would be generic and
>> the cover the SDIO case as well.
> 
> That sounds reasonable to me.
> 
> Addy: you've been very quiet.  What do you think?
> 
> -Doug
> 

Re: [PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Doug Anderson]

Hi,

On Thu, Jan 29, 2015 at 4:13 PM, Doug Anderson <dianders@chromium.org> wrote:
> I still don't exactly have a warm fuzzy about using the send_status()
> command like this, but it seems to work (actually, I should verify
> that myself--I've been taking Addy's word that his solution works).

So I finally started up a stress test.  I picked Addy's patch and
reverted my big fat delay.  Result was that Addy's patch doesn't seem
to hurt but once I revert my big fat delay then I seem to start
getting failures.  :(  I think it's actually on a different SD card
than the one that was originally causing failures, FWIW.

As usual with these kinds of tests, I'd like to test this a whole lot
more since it only reproduces sometimes, but unfortunately I'm going
to be unavailable for a few weeks starting from today, so I'm not sure
I'm not sure I can do more debugging / testing than what I've already
done...

...but unless using send_status allows us to remove the delay (or
unless there's some other justification for it) then it seems like we
should just drop this series...

Re: [PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[addy ke]

On 2015/1/30 09:08, addy ke wrote:
> hi, Doug
> 
> On 2015/1/30 08:13, Doug Anderson wrote:
>> Ulf,
>>
>> On Thu, Jan 29, 2015 at 1:16 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> - Drastically decreased cc-list.
>>>
>>> On 29 January 2015 at 01:55, Doug Anderson <dianders@chromium.org> wrote:
>>>> Ulf,
>>>>
>>>> On Tue, Jan 27, 2015 at 7:18 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>>> I asked Addy to post upstream against mmc_send_tuning(), but I guess
>>>>>> he didn't (he posted against Alex's NAKed patch instead).
>>>>>>
>>>>>> ...when I talked to him about it, Addy was asserting that when tuning
>>>>>> fails it is important (at least on dw_mmc on rk3288) that we wait for
>>>>>> the card to stop being busy and that the way to detect was using
>>>>>> mmc_send_status().
>>>>>
>>>>> So, could that be due to the internal logic of the error handling in
>>>>> dw_mmc driver? Or you think this is a generic issue?
>>>>>
>>>>> According to the specifications (eMMC and SD) both states that the
>>>>> tuning command has an R1 response. So, there shouldn't be any busy
>>>>> signalling involved - at least according to spec.
>>>>
>>>> I did a bit of digging into this issue myself.  What I found was that
>>>> a "response CRC" and "end of transfer".  This was why I posted up
>>>> <https://patchwork.kernel.org/patch/5623071/>.  From that patch:
>>>>
>>>>> Specifically it looks like in certain error conditions (I saw this
>>>>> with Response CRC errors) that data keeps showing up in the FIFO even
>>>>> after the error is reported and the CD (command done) bit is set.  If
>>>>> we don't wait for this data to finish transferring then it confuses
>>>>> the next transaction.  In the specific failure case I ran into I found
>>>>> that I could monitor the data_state_mc_busy bit and wait for it to
>>>>> clear, but in other failure cases this bit was stuck at busy when we
>>>>> saw an error.  Hence a generic big delay seems like the only option.
>>>
>>> I haven't queued that patch, I was waiting for an ack from Seungwon or Jaehoon.
>>>
>>> Do you think it could solve this issue, we could give it a try!?
>>
>> My big fat delay does seem to solve the issue, but it has the side
>> effect of slowing down tuning quite a bit so I'd rather find a more
>> proper fix.  We're talking several hundred extra milliseconds slower
>> per slot that is tuned...
>>
>> I still don't exactly have a warm fuzzy about using the send_status()
>> command like this, but it seems to work (actually, I should verify
>> that myself--I've been taking Addy's word that his solution works).  I
>> do wish someone could tell me "oh right, yeah, we do need a
>> send_status in that case".  ;)  Addy said that in the non-tuning case
>> that the core will always do a send_status so that this fix is really
>> only for tuning and doesn't need to be applied in general.  I also
>> haven't validated that myself...
>>
>> Overall it does sorta seem like this might just be a quirk with the
>> rk3288 dw_mmc.  It feels like the controller is in a wonky state and
>> maybe this extra send_status helps it get out?
>>
>>
>>>> ...Addy instead fixed the problem using mmc_send_status() to try to
>>>> detect when the transfer was all done and it apparently worked, but it
>>>> seemed odd to me.  My MMC "expertise" pretty much ends with looking
>>>> for simple logic errors in the MMC driver, so my hope was that one of
>>>> you guys would know this better...
>>>>
>>>>
>>>>>> That would mean that against upstream you'd need to change
>>>>>> mmc_send_tuning() to take in the card as well (or move the "host->card
>>>>>> = card" assignment to before UHS init, which seems less desirable?)
>>>
>>> I get your point now.
>>>
>>> Changing mmc_send_tuning() to take "card" will work due to $subject
>>> patch changed the ->execute_tuning() callbacks to take "card" as well.
>>>
>>>>>>
>>>>>> What do you think about that?  Is there a better solution?
>>>>>
>>>>> Why do we need to change mmc_send_tuning()? I thought the issue was
>>>>> that mmc_send_status(), which currently takes "card" as a parameter.
>>>>
>>>> Well, if mmc_send_tuning() needed to call mmc_send_status() then
>>>> mmc_send_tuning() would need the card parameter, right?
>>>
>>> Correct, got it now. :-)
>>>
>>> I didn't understand that you wanted mmc_send_tuning() to invoke
>>> mmc_send_status() while it got some errors. From Addy's patch2,
>>> mmc_send_status() is invoked from the host driver.
>>>
>>> Anyway, I think we should follow your suggestion to change the
>>> behaviour of mmc_send_tuning(). Though, I think it should use
>>> bus_ops->alive() callback instead (and that callback then also need to
>>> change to take "card" as a parameter), since that would be generic and
>>> the cover the SDIO case as well.
>>
>> That sounds reasonable to me.
>>
>> Addy: you've been very quiet.  What do you think?
> Sorry for reply late.
> I am busy with some other important things, and can't confirm it by pink2 board.
> 
> about bus_ops->alive, I think it can't use in tuning state.
> Because:
> bus_ops->alive() --> mmc_sd_alive(host) /* sd card */ -->mmc_send_status(host->card, NULL);
> But host->card == NULL in tuning state(mmc_sd_init_card->mmc_sd_init_uhs_card).
> 
> Only if sd is initialized successfully, we can get card pointer by host->card.
> see: mmc_sd_init_card(drivers/mmc/core/sd.c), the end of this function: host->card = card
And bus_ops->alive only check whether mmc is alive or not, the second parameter(*status) is NULL,
We can not get the card status.
But in tuning state, we need wait until card is idle, if the previous tuning is failed.

>>
>> -Doug
>>
>>
>>

Re: [PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Ulf Hansson]

>>
>> about bus_ops->alive, I think it can't use in tuning state.
>> Because:
>> bus_ops->alive() --> mmc_sd_alive(host) /* sd card */ -->mmc_send_status(host->card, NULL);
>> But host->card == NULL in tuning state(mmc_sd_init_card->mmc_sd_init_uhs_card).
>>
>> Only if sd is initialized successfully, we can get card pointer by host->card.
>> see: mmc_sd_init_card(drivers/mmc/core/sd.c), the end of this function: host->card = card
> And bus_ops->alive only check whether mmc is alive or not, the second parameter(*status) is NULL,
> We can not get the card status.
> But in tuning state, we need wait until card is idle, if the previous tuning is failed.

You are right that we can't use bus_ops->alive() in its current form.
Changing it to take "card" and "status" as parameter should fix this
for us. My point was more that we can't use mmc_send_status() since
that doesn't work for SDIO.

Anyway, it seems like we need to put this patchset on hold for a while.

You I merge the below patch instead so we at least have something
working for 3.20?

[PATCH] mmc: dw_mmc: rockchip: Add DW_MCI_QUIRK_RETRY_DELAY
https://lkml.org/lkml/2015/1/13/562

Kind regards
Uffe

Re: [PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Ulf Hansson]

On 2 February 2015 at 09:16, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>
>>> about bus_ops->alive, I think it can't use in tuning state.
>>> Because:
>>> bus_ops->alive() --> mmc_sd_alive(host) /* sd card */ -->mmc_send_status(host->card, NULL);
>>> But host->card == NULL in tuning state(mmc_sd_init_card->mmc_sd_init_uhs_card).
>>>
>>> Only if sd is initialized successfully, we can get card pointer by host->card.
>>> see: mmc_sd_init_card(drivers/mmc/core/sd.c), the end of this function: host->card = card
>> And bus_ops->alive only check whether mmc is alive or not, the second parameter(*status) is NULL,
>> We can not get the card status.
>> But in tuning state, we need wait until card is idle, if the previous tuning is failed.
>
> You are right that we can't use bus_ops->alive() in its current form.
> Changing it to take "card" and "status" as parameter should fix this
> for us. My point was more that we can't use mmc_send_status() since
> that doesn't work for SDIO.
>
> Anyway, it seems like we need to put this patchset on hold for a while.
>
> You I merge the below patch instead so we at least have something

/s /You / Should

> working for 3.20?
>
> [PATCH] mmc: dw_mmc: rockchip: Add DW_MCI_QUIRK_RETRY_DELAY
> https://lkml.org/lkml/2015/1/13/562
>
> Kind regards
> Uffe

Re: [PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[addy ke]

On 2015/2/2 16:17, Ulf Hansson wrote:
> On 2 February 2015 at 09:16, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>
>>>> about bus_ops->alive, I think it can't use in tuning state.
>>>> Because:
>>>> bus_ops->alive() --> mmc_sd_alive(host) /* sd card */ -->mmc_send_status(host->card, NULL);
>>>> But host->card == NULL in tuning state(mmc_sd_init_card->mmc_sd_init_uhs_card).
>>>>
>>>> Only if sd is initialized successfully, we can get card pointer by host->card.
>>>> see: mmc_sd_init_card(drivers/mmc/core/sd.c), the end of this function: host->card = card
>>> And bus_ops->alive only check whether mmc is alive or not, the second parameter(*status) is NULL,
>>> We can not get the card status.
>>> But in tuning state, we need wait until card is idle, if the previous tuning is failed.
>>
>> You are right that we can't use bus_ops->alive() in its current form.
>> Changing it to take "card" and "status" as parameter should fix this
>> for us. My point was more that we can't use mmc_send_status() since
>> that doesn't work for SDIO.

For sdio, I think maybe we can use CMD7 to get sdio status.

And there are 3 file which need get card status at least:
1. drivers/mmc/core/mmc_ops.c: mmc_send_status()
2. drivers/mmc/card/block.c: get_card_status()
3. drivers/mmc/card/mmc_test.c: mmc_test_wait_busy()
Maybe we need merge them and provide uniform interface for them.

>>
>> Anyway, it seems like we need to put this patchset on hold for a while.
>>
>> You I merge the below patch instead so we at least have something
> 
> /s /You / Should
> 
>> working for 3.20?
This patch can work, but it need delay 10ms each tuning.
It is too slow to initialize the card(tuning time >= (10 * tuning_count) ms)
>>
>> [PATCH] mmc: dw_mmc: rockchip: Add DW_MCI_QUIRK_RETRY_DELAY
>> https://lkml.org/lkml/2015/1/13/562
>>
>> Kind regards
>> Uffe
> 
> 
> 

Re: [PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Ulf Hansson]

On 2 February 2015 at 10:02, addy ke <addy.ke@rock-chips.com> wrote:
>
>
> On 2015/2/2 16:17, Ulf Hansson wrote:
>> On 2 February 2015 at 09:16, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>>
>>>>> about bus_ops->alive, I think it can't use in tuning state.
>>>>> Because:
>>>>> bus_ops->alive() --> mmc_sd_alive(host) /* sd card */ -->mmc_send_status(host->card, NULL);
>>>>> But host->card == NULL in tuning state(mmc_sd_init_card->mmc_sd_init_uhs_card).
>>>>>
>>>>> Only if sd is initialized successfully, we can get card pointer by host->card.
>>>>> see: mmc_sd_init_card(drivers/mmc/core/sd.c), the end of this function: host->card = card
>>>> And bus_ops->alive only check whether mmc is alive or not, the second parameter(*status) is NULL,
>>>> We can not get the card status.
>>>> But in tuning state, we need wait until card is idle, if the previous tuning is failed.
>>>
>>> You are right that we can't use bus_ops->alive() in its current form.
>>> Changing it to take "card" and "status" as parameter should fix this
>>> for us. My point was more that we can't use mmc_send_status() since
>>> that doesn't work for SDIO.
>
> For sdio, I think maybe we can use CMD7 to get sdio status.
>
> And there are 3 file which need get card status at least:
> 1. drivers/mmc/core/mmc_ops.c: mmc_send_status()
> 2. drivers/mmc/card/block.c: get_card_status()
> 3. drivers/mmc/card/mmc_test.c: mmc_test_wait_busy()
> Maybe we need merge them and provide uniform interface for them.
>
>>>
>>> Anyway, it seems like we need to put this patchset on hold for a while.
>>>
>>> You I merge the below patch instead so we at least have something
>>
>> /s /You / Should
>>
>>> working for 3.20?
> This patch can work, but it need delay 10ms each tuning.
> It is too slow to initialize the card(tuning time >= (10 * tuning_count) ms)

Yes, it seems like it's not the perfect solution, but what options do
we have right now? Leave it as is or should I apply below patch?

Jaehoon, what's your view on this?

>>>
>>> [PATCH] mmc: dw_mmc: rockchip: Add DW_MCI_QUIRK_RETRY_DELAY
>>> https://lkml.org/lkml/2015/1/13/562
>>>

Kind regards
Uffe

Re: [PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Jaehoon Chung]

On 02/04/2015 09:54 PM, Ulf Hansson wrote:
> On 2 February 2015 at 10:02, addy ke <addy.ke@rock-chips.com> wrote:
>>
>>
>> On 2015/2/2 16:17, Ulf Hansson wrote:
>>> On 2 February 2015 at 09:16, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>>>
>>>>>> about bus_ops->alive, I think it can't use in tuning state.
>>>>>> Because:
>>>>>> bus_ops->alive() --> mmc_sd_alive(host) /* sd card */ -->mmc_send_status(host->card, NULL);
>>>>>> But host->card == NULL in tuning state(mmc_sd_init_card->mmc_sd_init_uhs_card).
>>>>>>
>>>>>> Only if sd is initialized successfully, we can get card pointer by host->card.
>>>>>> see: mmc_sd_init_card(drivers/mmc/core/sd.c), the end of this function: host->card = card
>>>>> And bus_ops->alive only check whether mmc is alive or not, the second parameter(*status) is NULL,
>>>>> We can not get the card status.
>>>>> But in tuning state, we need wait until card is idle, if the previous tuning is failed.
>>>>
>>>> You are right that we can't use bus_ops->alive() in its current form.
>>>> Changing it to take "card" and "status" as parameter should fix this
>>>> for us. My point was more that we can't use mmc_send_status() since
>>>> that doesn't work for SDIO.
>>
>> For sdio, I think maybe we can use CMD7 to get sdio status.
>>
>> And there are 3 file which need get card status at least:
>> 1. drivers/mmc/core/mmc_ops.c: mmc_send_status()
>> 2. drivers/mmc/card/block.c: get_card_status()
>> 3. drivers/mmc/card/mmc_test.c: mmc_test_wait_busy()
>> Maybe we need merge them and provide uniform interface for them.
>>
>>>>
>>>> Anyway, it seems like we need to put this patchset on hold for a while.
>>>>
>>>> You I merge the below patch instead so we at least have something
>>>
>>> /s /You / Should
>>>
>>>> working for 3.20?
>> This patch can work, but it need delay 10ms each tuning.
>> It is too slow to initialize the card(tuning time >= (10 * tuning_count) ms)
> 
> Yes, it seems like it's not the perfect solution, but what options do
> we have right now? Leave it as is or should I apply below patch?
> 
> Jaehoon, what's your view on this?

It's not the best solution, but if needs to use this, we need to apply the other approach in future.
Well, i'm working other job..I think i can start to work for dw-mmc on next-week.
I think good that the below patch should be merged at first.
Then try to find and discuss more generic solution, how about?

Best Regards,
Jaehoon Chung

> 
>>>>
>>>> [PATCH] mmc: dw_mmc: rockchip: Add DW_MCI_QUIRK_RETRY_DELAY
>>>> https://lkml.org/lkml/2015/1/13/562
>>>>
> 
> Kind regards
> Uffe
>