[PATCH v4 0/4] UHS-I SDR-104 support for sh_mobile_sdhi

[PATCH v4 0/4] UHS-I SDR-104 support for sh_mobile_sdhi

[Simon Horman]

Hi,

this series is based on work by Ai Kyuse to add UHS-I SDR-104 support for
sh_mobile_sdhi. It builds on work by Shinobu Uehara, Rob Taylor, William
Towle and Ian Molton, Ben Hutchings, Wolfram Sang and others to add UHS-I
SDR-50 support to the same driver.

This series enables UHS-I SDR-104 on the r8a7790/Lager where
UHS-I SDR-50 is already enabled.

It is based on the next branch of the mmc tree

The first three mmc driver patches are targeted at the next branch of the mmc
tree. The last DT patch is targeted at the devel branch of the renesas tree.

To aid review this series is provided in the temporary topic/sdr104-v4 branch
of the renesas tree.

Changes since previous versions are described in individual patches.

In this patchset I have attempted to address all review received
for the v3 patch-set. I also believe that I have resolved a problem
where tuning would timeout under some circumstances: this seems to
have been due to several bugs introduced between v1 and v3.


Please see http://elinux.org/Tests:SD-SDHI-SDR104 for indicative tests
results. Highlights follow:

* Currently I am seeing speeds of up to 48MB/s with these patches and
  30MB/s without using a SanDisk Extreme Pro 8Gb microSDHC UHS-1 card.

* I am also seeing 45MB/s with these patches and 34MB/s without using
  a SanDisk Ultra 64Gb microSDXC UHS-1 card.

The above results are the same as previously reported for v2 and v3 of
this patchset.

Ai Kyuse (2):
  mmc: tmio: enhance illegal sequence handling
  mmc: tmio: Add tuning support

Simon Horman (2):
  mmc: sh_mobile_sdhi: Add tuning support
  ARM: dts: r8a7790: lager: Enable UHS-I SDR-104

 arch/arm/boot/dts/r8a7790-lager.dts |   1 +
 drivers/mmc/host/sh_mobile_sdhi.c   | 264 +++++++++++++++++++++++++++++++++++-
 drivers/mmc/host/tmio_mmc.h         |   7 +
 drivers/mmc/host/tmio_mmc_pio.c     |  92 ++++++++++++-
 4 files changed, 357 insertions(+), 7 deletions(-)

-- 
2.7.0.rc3.207.g0ac5344

[PATCH v4 1/4] mmc: tmio: enhance illegal sequence handling

[Simon Horman]

From: Ai Kyuse <ai.kyuse.uw@renesas.com>

An illegal sequence command error may occur if there is a stopbit or
cmd_index error as well as a CRC error. The correct course of action
is to re-enable IRQs

An illegal sequence data error may occur if there is a CRC or stopbit
error,  or underrun. In this case set data->error correctly.

This is in preparation for enabling tuning support which relies on
differentiating between illegal sequence and other errors.

Signed-off-by: Ai Kyuse <ai.kyuse.uw@renesas.com>
[simon: broken out of a larger patch]
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
v4
* Added Ack from Wolfram Sang

v3
* No change

v2
* Broken out of a larger patch (but forgot to post)
---
 drivers/mmc/host/tmio_mmc_pio.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index 92467efc4e2c..a9d07b5f3c63 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -520,7 +520,7 @@ void tmio_mmc_do_data_irq(struct tmio_mmc_host *host)
 	schedule_work(&host->done);
 }
 
-static void tmio_mmc_data_irq(struct tmio_mmc_host *host)
+static void tmio_mmc_data_irq(struct tmio_mmc_host *host, unsigned int stat)
 {
 	struct mmc_data *data;
 	spin_lock(&host->lock);
@@ -529,6 +529,9 @@ static void tmio_mmc_data_irq(struct tmio_mmc_host *host)
 	if (!data)
 		goto out;
 
+	if (stat & TMIO_STAT_CRCFAIL || stat & TMIO_STAT_STOPBIT_ERR ||
+	    stat & TMIO_STAT_TXUNDERRUN)
+		data->error = -EILSEQ;
 	if (host->chan_tx && (data->flags & MMC_DATA_WRITE) && !host->force_pio) {
 		u32 status = sd_ctrl_read16_and_16_as_32(host, CTL_STATUS);
 		bool done = false;
@@ -577,8 +580,6 @@ static void tmio_mmc_cmd_irq(struct tmio_mmc_host *host,
 		goto out;
 	}
 
-	host->cmd = NULL;
-
 	/* This controller is sicker than the PXA one. Not only do we need to
 	 * drop the top 8 bits of the first response word, we also need to
 	 * modify the order of the response for short response command types.
@@ -598,14 +599,16 @@ static void tmio_mmc_cmd_irq(struct tmio_mmc_host *host,
 
 	if (stat & TMIO_STAT_CMDTIMEOUT)
 		cmd->error = -ETIMEDOUT;
-	else if (stat & TMIO_STAT_CRCFAIL && cmd->flags & MMC_RSP_CRC)
+	else if ((stat & TMIO_STAT_CRCFAIL && cmd->flags & MMC_RSP_CRC) ||
+		 stat & TMIO_STAT_STOPBIT_ERR ||
+		 stat & TMIO_STAT_CMD_IDX_ERR)
 		cmd->error = -EILSEQ;
 
 	/* If there is data to handle we enable data IRQs here, and
 	 * we will ultimatley finish the request in the data_end handler.
 	 * If theres no data or we encountered an error, finish now.
 	 */
-	if (host->data && !cmd->error) {
+	if (host->data && (!cmd->error || cmd->error == -EILSEQ)) {
 		if (host->data->flags & MMC_DATA_READ) {
 			if (host->force_pio || !host->chan_rx)
 				tmio_mmc_enable_mmc_irqs(host, TMIO_MASK_READOP);
@@ -666,7 +669,7 @@ static bool __tmio_mmc_sdcard_irq(struct tmio_mmc_host *host,
 	/* Data transfer completion */
 	if (ireg & TMIO_STAT_DATAEND) {
 		tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_DATAEND);
-		tmio_mmc_data_irq(host);
+		tmio_mmc_data_irq(host, status);
 		return true;
 	}
 
-- 
2.7.0.rc3.207.g0ac5344

[PATCH v4 2/4] mmc: tmio: Add tuning support

[Simon Horman]

From: Ai Kyuse <ai.kyuse.uw@renesas.com>

Add tuning support for use with SDR104 mode

Signed-off-by: Ai Kyuse <ai.kyuse.uw@renesas.com>
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
v4 [Simon Horman]
As suggested by Wolfram Sang:
  - Do not perform tuning if host->select_tuning is not set:
    it seems to make little sense to do so and moreover there is currently
    no such use-case
  - Do not add mrc->sbc handling from tmio_mmc_request,
    this is a hang-over from earlier versions of this patchset which
    did not use core infrastructure for retuning
  - Tidy up local variable usage
* Correct index passed to prepare_tuning(): this seems to have
  been the last piece of resolving the timeouts during tuning puzzle
* Further cleanups to tmio_mmc_execute_tuning():
  - Ensure tap is sized proportionally to its members
  - Remove stray '*' in comment
  - Use mmc rather than host->mmc, these are equivalent but
    the former seems tidier
  - Correct inverted logic in setting tap values
* Re-introduce retuning support. This was removed in v3.

v3 [Simon Horman]
* As suggested by Kuninori Morimoto:
  - Do not add unused retuning callback to struct tmio_mmc_host
  - Change return type of prepare_tuning callback to void
  - Add tap_size parameter to select_tuning callback

v2 [Simon Horman]
* As suggested by Kuninori Morimoto:
  - Actually remove unnecessary TMIO_MMC_HAS_UHS_SCC define
* As suggested by Wolfram Sang:
  - Rely on core to call tuning. This simplifies things somewhat.
  - Use mmc_send_tuning()
    - A side affect of this appears to be that we now see some recoverable
      errors logged during tuning. These are typically corrected by
      subsequent tuning. It is the logging that is the apparent side effect
      of this change.
      e.g.
      sh_mobile_sdhi ee100000.sd: timeout waiting for hardware interrupt (CMD19)
      sh_mobile_sdhi ee100000.sd: Tuning procedure failed
* Use bool rather than unsigned long to pass test status
  to select_tuning() callback
* Do not retune if init_tuning callback is not present or
  indicates that there are no taps present
* Retune on hardware reset

v1 [Simon Horman]
* Omit start_signal_voltage_switch and tmio_mmc_card_busy changes which are
  already present in mainline in a different form
* Return num from init_tuning rather than passing an extra parameter
  to hold the return value
* Only call host->init_tuning if it is non-NULL
* Place tmio_mmc_execute_tuning() such that no new forward declarations are
  required
* Remove unused TMIO_MMC_HAS_UHS_SCC define

v0 [Ai Kyuse]

Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
 drivers/mmc/host/tmio_mmc.h     |  7 ++++
 drivers/mmc/host/tmio_mmc_pio.c | 77 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)

diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 7f63ec05bdf4..316b0c3fe745 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -150,6 +150,7 @@ struct tmio_mmc_host {
 	struct mutex		ios_lock;	/* protect set_ios() context */
 	bool			native_hotplug;
 	bool			sdio_irq_enabled;
+	u32			scc_tappos;
 
 	int (*write16_hook)(struct tmio_mmc_host *host, int addr);
 	int (*clk_enable)(struct tmio_mmc_host *host);
@@ -160,6 +161,12 @@ struct tmio_mmc_host {
 			      unsigned int direction, int blk_size);
 	int (*start_signal_voltage_switch)(struct mmc_host *mmc,
 					   struct mmc_ios *ios);
+	unsigned int (*init_tuning)(struct tmio_mmc_host *host);
+	void (*prepare_tuning)(struct tmio_mmc_host *host, unsigned long tap);
+	int (*select_tuning)(struct tmio_mmc_host *host, bool *tap,
+			     int tap_size);
+	bool (*retuning)(struct tmio_mmc_host *host);
+	void (*hw_reset)(struct tmio_mmc_host *host);
 };
 
 struct tmio_mmc_host *tmio_mmc_host_alloc(struct platform_device *pdev);
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index a9d07b5f3c63..83b5148a2684 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -36,6 +36,7 @@
 #include <linux/io.h>
 #include <linux/irq.h>
 #include <linux/mfd/tmio.h>
+#include <linux/mmc/card.h>
 #include <linux/mmc/host.h>
 #include <linux/mmc/mmc.h>
 #include <linux/mmc/slot-gpio.h>
@@ -298,6 +299,15 @@ static void tmio_mmc_finish_request(struct tmio_mmc_host *host)
 	if (mrq->cmd->error || (mrq->data && mrq->data->error))
 		tmio_mmc_abort_dma(host);
 
+	if (host->retuning) {
+		int result = host->retuning(host);
+
+		if (result || (mrq->cmd->error == -EILSEQ)) {
+			mmc_retune_timer_stop(host->mmc);
+			mmc_retune_needed(host->mmc);
+		}
+	}
+
 	mmc_request_done(host->mmc, mrq);
 }
 
@@ -756,6 +766,68 @@ static int tmio_mmc_start_data(struct tmio_mmc_host *host,
 	return 0;
 }
 
+static void tmio_mmc_hw_reset(struct mmc_host *mmc)
+{
+	struct tmio_mmc_host *host = mmc_priv(mmc);
+
+	if (host->hw_reset)
+		host->hw_reset(host);
+
+	mmc_retune_timer_stop(host->mmc);
+	mmc_retune_needed(host->mmc);
+}
+
+static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
+{
+	struct tmio_mmc_host *host = mmc_priv(mmc);
+	unsigned int num;
+	int i, ret = 0;
+	bool *tap;
+
+	if (!host->init_tuning || !host->select_tuning)
+		/* Tuning is not supported */
+		goto out;
+
+	num = host->init_tuning(host);
+	if (!num)
+		/* Tuning is not supported */
+		goto out;
+
+	tap = kmalloc(num * 2 * sizeof(*tap), GFP_KERNEL);
+	if (!tap) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	/* Issue CMD19 twice for each tap */
+	for (i = 0; i < 2 * num; i++) {
+		if (host->prepare_tuning)
+			host->prepare_tuning(host, i % num);
+
+		ret = mmc_send_tuning(mmc, opcode, NULL);
+		if (ret && ret != -EILSEQ)
+			goto err_free;
+		tap[i] = (ret != 0);
+
+		mdelay(1);
+	}
+
+	ret = host->select_tuning(host, tap, num * 2);
+
+err_free:
+	kfree(tap);
+out:
+	if (ret < 0) {
+		dev_warn(&host->pdev->dev, "Tuning procedure failed\n");
+		tmio_mmc_hw_reset(mmc);
+	} else {
+		host->mmc->retune_period = 0;
+	}
+
+	return ret;
+
+}
+
 /* Process requests from the MMC layer */
 static void tmio_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
 {
@@ -978,6 +1050,8 @@ static struct mmc_host_ops tmio_mmc_ops = {
 	.enable_sdio_irq = tmio_mmc_enable_sdio_irq,
 	.card_busy	= tmio_mmc_card_busy,
 	.multi_io_quirk	= tmio_multi_io_quirk,
+	.execute_tuning = tmio_mmc_execute_tuning,
+	.hw_reset	= tmio_mmc_hw_reset,
 };
 
 static int tmio_mmc_init_ocr(struct tmio_mmc_host *host)
@@ -1202,6 +1276,9 @@ int tmio_mmc_host_runtime_suspend(struct device *dev)
 	struct mmc_host *mmc = dev_get_drvdata(dev);
 	struct tmio_mmc_host *host = mmc_priv(mmc);
 
+	mmc_retune_timer_stop(host->mmc);
+	mmc_retune_needed(host->mmc);
+
 	tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL);
 
 	if (host->clk_cache)
-- 
2.7.0.rc3.207.g0ac5344

[PATCH v4 3/4] mmc: sh_mobile_sdhi: Add tuning support

[Simon Horman]

Add tuning support for use with SDR104 mode
This includes adding support for the sampling clock controller (SCC).

Based on work by Ai Kyuse.

Cc: Ai Kyuse <ai.kyuse.uw@renesas.com>
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
v4 [Simon Horman]

As suggested by Geert Uytterhoeven:
* guard SDR104 specific portion of probe with host->mmc->caps &
  MMC_CAP_UHS_SDR104

* As suggested by Wolfram Sang:
  - Pass priv to sd_scc_{read,write}32 to save host_to_priv access
    for each call
  - Use 0x0 instead of other representations of 0 in hex
  - Use CLK_CTL_SCLKEN
  - Do not add unused SH_MOBILE_SDHI_MAX_TAP
  - Use ternary operator in sh_mobile_sdhi_select_tuning
  - Do include as yet unsupported HS200 in error string
* Reintroduce retuning support: This was removed in v3.
* Revert to algorithm in v1 patchset, on further reading of the documentation
  it appears to be correct.

v3 [Simon Horman]
* As suggested by Kuninori Morimoto:
  - Do not add unused retuning callback to struct tmio_mmc_host
  - Change return type of prepare_tuning callback to void
  - Add tap_size parameter to select_tuning callback

v2 [Simon Horman]
* As suggested by Kuninori Morimoto
  - Use host->mmc->caps & MMC_CAP_UHS_SDR104 instead of
    pdata->flags & TMIO_MMC_HAS_UHS_SCC to avoid needing the
    MMC_CAP_UHS_SDR104 flag at all.
    N.B: Morimoto-san suggested using but this flag is not actually
    set there in by current probe come.
  - Simplify logic in sh_mobile_sdhi_inquiry_tuning
* As suggested by Wolfram Sang
  - Use clk_rate instead of clk for field in struct sh_mobile_sdhi_scc
  - Remove inquiry_tuning callback which is now unnecessary as calling
    of tuning is handled by the core
  - Removed unused sh_mobile_sdhi_set_clk_div callback
  - Save sci_base address rather than calculating it on each read and write
* Update selection logic in sh_mobile_sdhi_select_tuning to match spec
* Use bool instead of long for taps parameter of
  sh_mobile_sdhi_select_tuning()
* Return 0 from sh_mobile_sdhi_init_tuning() if the
  SDR104 capability is not set and thus tuning should not
  be performed because it is not supported by the hardware

v1 [Simon Horman]
* Rebase
* Always use value of 0x8 for TAPNUM field of DTCNTL register
  rather than reading value from DT property. There does not
  seem to be a need to expose this in DT at this point.
* Do not include tmio_mmc_start_signal_voltage_switch changes which
  are already in mainline in a different form
* Do not add renesas,clk-rate property as the max-frequency property, which
  is now present in mainline, seems to provide the needed rate
* Omit Gen3 specific changes
* Do not provide renesas,mmc-scc-tappos DT property.
  Instead, always use taps provided in driver.
* Do not parse sd-uhs-sdr50 and sd-uhs-sdr104 properties.
  This is handled by the core.

v0 [Ai Kyuse]

Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
 drivers/mmc/host/sh_mobile_sdhi.c | 264 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 263 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index c3b651bf89cb..ff882fb41743 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -47,6 +47,11 @@
 
 #define host_to_priv(host) container_of((host)->pdata, struct sh_mobile_sdhi, mmc_data)
 
+struct sh_mobile_sdhi_scc {
+	unsigned long clk_rate;	/* clock rate for SDR104 */
+	u32 tap;		/* sampling clock position for SDR104 */
+};
+
 struct sh_mobile_sdhi_of_data {
 	unsigned long tmio_flags;
 	unsigned long capabilities;
@@ -54,6 +59,9 @@ struct sh_mobile_sdhi_of_data {
 	enum dma_slave_buswidth dma_buswidth;
 	dma_addr_t dma_rx_offset;
 	unsigned bus_shift;
+	int scc_offset;
+	struct sh_mobile_sdhi_scc *taps;
+	int taps_num;
 };
 
 static const struct sh_mobile_sdhi_of_data of_default_cfg = {
@@ -66,12 +74,27 @@ static const struct sh_mobile_sdhi_of_data of_rcar_gen1_compatible = {
 	.capabilities	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ,
 };
 
+/* Definitions for sampling clocks */
+static struct sh_mobile_sdhi_scc rcar_gen2_scc_taps[] = {
+	{
+		.clk_rate = 156000000,
+		.tap = 0x00000703,
+	},
+	{
+		.clk_rate = 0,
+		.tap = 0x00000300,
+	},
+};
+
 static const struct sh_mobile_sdhi_of_data of_rcar_gen2_compatible = {
 	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_WRPROTECT_DISABLE |
 			  TMIO_MMC_CLK_ACTUAL | TMIO_MMC_MIN_RCAR2,
 	.capabilities	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ,
 	.dma_buswidth	= DMA_SLAVE_BUSWIDTH_4_BYTES,
 	.dma_rx_offset	= 0x2000,
+	.scc_offset	= 0x0300,
+	.taps		= rcar_gen2_scc_taps,
+	.taps_num	= ARRAY_SIZE(rcar_gen2_scc_taps),
 };
 
 static const struct sh_mobile_sdhi_of_data of_rcar_gen3_compatible = {
@@ -104,6 +127,7 @@ struct sh_mobile_sdhi {
 	struct tmio_mmc_dma dma_priv;
 	struct pinctrl *pinctrl;
 	struct pinctrl_state *pins_default, *pins_uhs;
+	void __iomem *scc_ctl;
 };
 
 static void sh_mobile_sdhi_sdbuf_width(struct tmio_mmc_host *host, int width)
@@ -247,6 +271,211 @@ static int sh_mobile_sdhi_start_signal_voltage_switch(struct mmc_host *mmc,
 	return pinctrl_select_state(priv->pinctrl, pin_state);
 }
 
+/* SCC registers */
+#define SH_MOBILE_SDHI_SCC_DTCNTL	0x000
+#define SH_MOBILE_SDHI_SCC_TAPSET	0x002
+#define SH_MOBILE_SDHI_SCC_DT2FF	0x004
+#define SH_MOBILE_SDHI_SCC_CKSEL	0x006
+#define SH_MOBILE_SDHI_SCC_RVSCNTL	0x008
+#define SH_MOBILE_SDHI_SCC_RVSREQ	0x00A
+
+/* Definitions for values the SH_MOBILE_SDHI_SCC_DTCNTL register */
+#define SH_MOBILE_SDHI_SCC_DTCNTL_TAPEN		BIT(0)
+#define SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_SHIFT	16
+#define SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_MASK	0xff
+
+/* Definitions for values the SH_MOBILE_SDHI_SCC_CKSEL register */
+#define SH_MOBILE_SDHI_SCC_CKSEL_DTSEL		BIT(0)
+/* Definitions for values the SH_MOBILE_SDHI_SCC_RVSCNTL register */
+#define SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN	BIT(1)
+/* Definitions for values the SH_MOBILE_SDHI_SCC_RVSREQ register */
+#define SH_MOBILE_SDHI_SCC_RVSREQ_RVSERR	BIT(2)
+
+static inline u32 sd_scc_read32(struct tmio_mmc_host *host,
+				struct sh_mobile_sdhi *priv, int addr)
+{
+	return readl(priv->scc_ctl + (addr << host->bus_shift));
+}
+
+static inline void sd_scc_write32(struct tmio_mmc_host *host,
+				  struct sh_mobile_sdhi *priv,
+				  int addr, u32 val)
+{
+	writel(val, priv->scc_ctl + (addr << host->bus_shift));
+}
+
+static unsigned int sh_mobile_sdhi_init_tuning(struct tmio_mmc_host *host)
+{
+	struct sh_mobile_sdhi *priv;
+
+	if (!(host->mmc->caps & MMC_CAP_UHS_SDR104))
+		return 0;
+
+	priv = host_to_priv(host);
+
+	/* set sampling clock selection range */
+	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_DTCNTL,
+		       0x8 << SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_SHIFT);
+
+	/* Initialize SCC */
+	sd_ctrl_write32_as_16_and_16(host, CTL_STATUS, 0x0);
+
+	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_DTCNTL,
+		       SH_MOBILE_SDHI_SCC_DTCNTL_TAPEN |
+		       sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_DTCNTL));
+
+	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, ~CLK_CTL_SCLKEN &
+			sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
+
+	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_CKSEL,
+		       SH_MOBILE_SDHI_SCC_CKSEL_DTSEL |
+		       sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_CKSEL));
+
+	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, CLK_CTL_SCLKEN |
+			sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
+
+	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL,
+		       ~SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN &
+		       sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL));
+
+	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_DT2FF, host->scc_tappos);
+
+	/* Read TAPNUM */
+	return (sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_DTCNTL) >>
+		SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_SHIFT) &
+		SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_MASK;
+}
+
+static void sh_mobile_sdhi_prepare_tuning(struct tmio_mmc_host *host,
+					 unsigned long tap)
+{
+	struct sh_mobile_sdhi *priv = host_to_priv(host);
+
+	/* Set sampling clock position */
+	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, tap);
+}
+
+#define SH_MOBILE_SDHI_MAX_TAP 3
+
+static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host,
+					bool *tap, int tap_size)
+{
+	struct sh_mobile_sdhi *priv = host_to_priv(host);
+	unsigned long tap_num;  /* total number of taps */
+	unsigned long tap_cnt;  /* counter of tuning success */
+	unsigned long tap_set;  /* tap position */
+	unsigned long tap_start;/* start position of tuning success */
+	unsigned long tap_end;  /* end position of tuning success */
+	unsigned long ntap;     /* temporary counter of tuning success */
+	unsigned long i;
+
+	/* Clear SCC_RVSREQ */
+	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSREQ, 0);
+
+	/* Select SCC */
+	tap_num = (sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_DTCNTL) >>
+		   SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_SHIFT) &
+		SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_MASK;
+
+	if (tap_num * 2 != tap_size)
+		return -EINVAL;
+
+	/*
+	 * Find the longest consecutive run of successful probes.  If that
+	 * is more than SH_MOBILE_SDHI_MAX_TAP probes long then use the
+	 * center index as the tap.
+	 */
+	tap_cnt = 0;
+	ntap = 0;
+	tap_start = 0;
+	tap_end = 0;
+	for (i = 0; i < tap_num * 2; i++) {
+		if (tap[i] == 0)
+			ntap++;
+		else {
+			if (ntap > tap_cnt) {
+				tap_start = i - ntap;
+				tap_end = i - 1;
+				tap_cnt = ntap;
+			}
+			ntap = 0;
+		}
+	}
+
+	if (ntap > tap_cnt) {
+		tap_start = i - ntap;
+		tap_end = i - 1;
+		tap_cnt = ntap;
+	}
+
+	if (tap_cnt >= SH_MOBILE_SDHI_MAX_TAP)
+		tap_set = (tap_start + tap_end) / 2 % tap_num;
+	else
+		return -EIO;
+
+	/* Set SCC */
+	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, tap_set);
+
+	/* Enable auto re-tuning */
+	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL,
+		       SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN |
+		       sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL));
+
+	return 0;
+}
+
+
+static bool sh_mobile_sdhi_retuning(struct tmio_mmc_host *host)
+{
+	struct sh_mobile_sdhi *priv;
+
+	if (!(host->mmc->caps & MMC_CAP_UHS_SDR104))
+		return 0;
+
+	priv = host_to_priv(host);
+
+	/* Check SCC error */
+	if (sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL) &
+	    SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN &&
+	    sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSREQ) &
+	    SH_MOBILE_SDHI_SCC_RVSREQ_RVSERR) {
+		/* Clear SCC error */
+		sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSREQ, 0);
+		return true;
+	}
+
+	return false;
+}
+
+static void sh_mobile_sdhi_hw_reset(struct tmio_mmc_host *host)
+{
+	struct sh_mobile_sdhi *priv;
+
+	if (host->mmc->caps & MMC_CAP_UHS_SDR104)
+		return 0;
+
+	priv = host_to_priv(host);
+
+	/* Reset SCC */
+	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, ~CLK_CTL_SCLKEN &
+			sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
+
+	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_CKSEL,
+		       ~SH_MOBILE_SDHI_SCC_CKSEL_DTSEL &
+		       sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_CKSEL));
+
+	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, CLK_CTL_SCLKEN |
+			sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
+
+	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL,
+		       ~SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN &
+		       sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL));
+
+	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL,
+		       ~SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN &
+		       sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL));
+}
+
 static int sh_mobile_sdhi_wait_idle(struct tmio_mmc_host *host)
 {
 	int timeout = 1000;
@@ -317,7 +546,7 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 	struct tmio_mmc_data *mmd = pdev->dev.platform_data;
 	struct tmio_mmc_host *host;
 	struct resource *res;
-	int irq, ret, i = 0;
+	int irq, ret, i;
 	struct tmio_mmc_dma *dma_priv;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -370,6 +599,11 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 	host->clk_disable	= sh_mobile_sdhi_clk_disable;
 	host->multi_io_quirk	= sh_mobile_sdhi_multi_io_quirk;
 	host->start_signal_voltage_switch = sh_mobile_sdhi_start_signal_voltage_switch;
+	host->init_tuning	= sh_mobile_sdhi_init_tuning;
+	host->prepare_tuning	= sh_mobile_sdhi_prepare_tuning;
+	host->select_tuning	= sh_mobile_sdhi_select_tuning;
+	host->retuning		= sh_mobile_sdhi_retuning;
+	host->hw_reset		= sh_mobile_sdhi_hw_reset;
 
 	/* Orginally registers were 16 bit apart, could be 32 or 64 nowadays */
 	if (!host->bus_shift && resource_size(res) > 0x100) /* old way to determine the shift */
@@ -409,6 +643,34 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto efree;
 
+	if (host->mmc->caps & MMC_CAP_UHS_SDR104) {
+		host->mmc->caps |= MMC_CAP_HW_RESET;
+
+		if (of_id && of_id->data) {
+			const struct sh_mobile_sdhi_of_data *of_data;
+			const struct sh_mobile_sdhi_scc *taps;
+			bool hit = false;
+
+			of_data = of_id->data;
+			taps = of_data->taps;
+
+			for (i = 0; i < of_data->taps_num; i++) {
+				if (taps[i].clk_rate == 0 ||
+				    taps[i].clk_rate == host->mmc->f_max) {
+					host->scc_tappos = taps->tap;
+					hit = true;
+					break;
+				}
+			}
+
+			if (!hit)
+				dev_warn(&host->pdev->dev, "Unknown clock rate for SDR104\n");
+
+			priv->scc_ctl = host->ctl + of_data->scc_offset;
+		}
+	}
+
+	i = 0;
 	while (1) {
 		irq = platform_get_irq(pdev, i);
 		if (irq < 0)
-- 
2.7.0.rc3.207.g0ac5344

[PATCH v4 4/4] ARM: dts: r8a7790: lager: Enable UHS-I SDR-104

[Simon Horman]

Add the sd-uhs-sdr104 property to SDHI0.

Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
 arch/arm/boot/dts/r8a7790-lager.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
index 749ba02b6a53..05d1ff7acee2 100644
--- a/arch/arm/boot/dts/r8a7790-lager.dts
+++ b/arch/arm/boot/dts/r8a7790-lager.dts
@@ -559,6 +559,7 @@
 	vqmmc-supply = <&vccq_sdhi0>;
 	cd-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>;
 	sd-uhs-sdr50;
+	sd-uhs-sdr104;
 	status = "okay";
 };
 
-- 
2.7.0.rc3.207.g0ac5344

Re: [PATCH v4 3/4] mmc: sh_mobile_sdhi: Add tuning support

[Simon Horman]

On Wed, Jul 27, 2016 at 01:13:22PM +0900, Simon Horman wrote:
> Add tuning support for use with SDR104 mode
> This includes adding support for the sampling clock controller (SCC).
> 
> Based on work by Ai Kyuse.
> 
> Cc: Ai Kyuse <ai.kyuse.uw@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>

...

> +static void sh_mobile_sdhi_hw_reset(struct tmio_mmc_host *host)
> +{
> +	struct sh_mobile_sdhi *priv;
> +
> +	if (host->mmc->caps & MMC_CAP_UHS_SDR104)
> +		return 0;

The above line should be:

		return;

Re: [PATCH v4 2/4] mmc: tmio: Add tuning support

[Wolfram Sang]

> +static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
> +{
> +	struct tmio_mmc_host *host = mmc_priv(mmc);
> +	unsigned int num;
> +	int i, ret = 0;
> +	bool *tap;
> +
> +	if (!host->init_tuning || !host->select_tuning)

Check host->prepare_tuning, too?

> +		/* Tuning is not supported */
> +		goto out;
> +
> +	num = host->init_tuning(host);
> +	if (!num)
> +		/* Tuning is not supported */
> +		goto out;
> +
> +	tap = kmalloc(num * 2 * sizeof(*tap), GFP_KERNEL);
> +	if (!tap) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	/* Issue CMD19 twice for each tap */
> +	for (i = 0; i < 2 * num; i++) {
> +		if (host->prepare_tuning)
> +			host->prepare_tuning(host, i % num);
> +
> +		ret = mmc_send_tuning(mmc, opcode, NULL);
> +		if (ret && ret != -EILSEQ)
> +			goto err_free;
> +		tap[i] = (ret != 0);
> +
> +		mdelay(1);
> +	}
> +
> +	ret = host->select_tuning(host, tap, num * 2);
> +
> +err_free:
> +	kfree(tap);
> +out:
> +	if (ret < 0) {
> +		dev_warn(&host->pdev->dev, "Tuning procedure failed\n");
> +		tmio_mmc_hw_reset(mmc);
> +	} else {
> +		host->mmc->retune_period = 0;
> +	}
> +
> +	return ret;
> +

Unnecessary blank line

Re: [PATCH v4 0/4] UHS-I SDR-104 support for sh_mobile_sdhi

[Wolfram Sang]

Hi Simon,

> In this patchset I have attempted to address all review received
> for the v3 patch-set. I also believe that I have resolved a problem
> where tuning would timeout under some circumstances: this seems to
> have been due to several bugs introduced between v1 and v3.

So, you didn't see timeouts anymore?

> * Currently I am seeing speeds of up to 48MB/s with these patches and
>   30MB/s without using a SanDisk Extreme Pro 8Gb microSDHC UHS-1 card.
> 
> * I am also seeing 45MB/s with these patches and 34MB/s without using
>   a SanDisk Ultra 64Gb microSDXC UHS-1 card.

I got similar numbers for my SanDisk and Samsung cards. While the SDR104
speed is not that what I hoped for, it is still an improvement over
SDR50. We are currently investigating if DMA causes a bottleneck
somewhere perhaps. Except for a minor comment, I'd think these patches
are good to go in and we could improve incrementally from here:

Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Maybe Ulf can have another high-level view on those. I guess he is more
experienced with the tuning stuff?

Thanks,

   Wolfram

Re: [PATCH v4 0/4] UHS-I SDR-104 support for sh_mobile_sdhi

[Simon Horman]

On Wed, Aug 10, 2016 at 03:12:24PM +0200, Wolfram Sang wrote:
> Hi Simon,
> 
> > In this patchset I have attempted to address all review received
> > for the v3 patch-set. I also believe that I have resolved a problem
> > where tuning would timeout under some circumstances: this seems to
> > have been due to several bugs introduced between v1 and v3.
> 
> So, you didn't see timeouts anymore?

Yes, that is correct.

I have not seen them at all with this patchset.
I will extend my testing as I integrate these changes on other SoCs and boards.

> > * Currently I am seeing speeds of up to 48MB/s with these patches and
> >   30MB/s without using a SanDisk Extreme Pro 8Gb microSDHC UHS-1 card.
> > 
> > * I am also seeing 45MB/s with these patches and 34MB/s without using
> >   a SanDisk Ultra 64Gb microSDXC UHS-1 card.
> 
> I got similar numbers for my SanDisk and Samsung cards. While the SDR104
> speed is not that what I hoped for, it is still an improvement over
> SDR50. We are currently investigating if DMA causes a bottleneck
> somewhere perhaps. Except for a minor comment, I'd think these patches
> are good to go in and we could improve incrementally from here:
> 
> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Maybe Ulf can have another high-level view on those. I guess he is more
> experienced with the tuning stuff?

Yes, of course such input would be welcome.
I'd like to work towards getting these changes merged.

Re: [PATCH v4 2/4] mmc: tmio: Add tuning support

[Ulf Hansson]

On 27 July 2016 at 06:13, Simon Horman <horms+renesas@verge.net.au> wrote:
> From: Ai Kyuse <ai.kyuse.uw@renesas.com>
>
> Add tuning support for use with SDR104 mode
>
> Signed-off-by: Ai Kyuse <ai.kyuse.uw@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> ---
> v4 [Simon Horman]
> As suggested by Wolfram Sang:
>   - Do not perform tuning if host->select_tuning is not set:
>     it seems to make little sense to do so and moreover there is currently
>     no such use-case
>   - Do not add mrc->sbc handling from tmio_mmc_request,
>     this is a hang-over from earlier versions of this patchset which
>     did not use core infrastructure for retuning
>   - Tidy up local variable usage
> * Correct index passed to prepare_tuning(): this seems to have
>   been the last piece of resolving the timeouts during tuning puzzle
> * Further cleanups to tmio_mmc_execute_tuning():
>   - Ensure tap is sized proportionally to its members
>   - Remove stray '*' in comment
>   - Use mmc rather than host->mmc, these are equivalent but
>     the former seems tidier
>   - Correct inverted logic in setting tap values
> * Re-introduce retuning support. This was removed in v3.
>
> v3 [Simon Horman]
> * As suggested by Kuninori Morimoto:
>   - Do not add unused retuning callback to struct tmio_mmc_host
>   - Change return type of prepare_tuning callback to void
>   - Add tap_size parameter to select_tuning callback
>
> v2 [Simon Horman]
> * As suggested by Kuninori Morimoto:
>   - Actually remove unnecessary TMIO_MMC_HAS_UHS_SCC define
> * As suggested by Wolfram Sang:
>   - Rely on core to call tuning. This simplifies things somewhat.
>   - Use mmc_send_tuning()
>     - A side affect of this appears to be that we now see some recoverable
>       errors logged during tuning. These are typically corrected by
>       subsequent tuning. It is the logging that is the apparent side effect
>       of this change.
>       e.g.
>       sh_mobile_sdhi ee100000.sd: timeout waiting for hardware interrupt (CMD19)
>       sh_mobile_sdhi ee100000.sd: Tuning procedure failed
> * Use bool rather than unsigned long to pass test status
>   to select_tuning() callback
> * Do not retune if init_tuning callback is not present or
>   indicates that there are no taps present
> * Retune on hardware reset
>
> v1 [Simon Horman]
> * Omit start_signal_voltage_switch and tmio_mmc_card_busy changes which are
>   already present in mainline in a different form
> * Return num from init_tuning rather than passing an extra parameter
>   to hold the return value
> * Only call host->init_tuning if it is non-NULL
> * Place tmio_mmc_execute_tuning() such that no new forward declarations are
>   required
> * Remove unused TMIO_MMC_HAS_UHS_SCC define
>
> v0 [Ai Kyuse]
>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> ---
>  drivers/mmc/host/tmio_mmc.h     |  7 ++++
>  drivers/mmc/host/tmio_mmc_pio.c | 77 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 84 insertions(+)
>
> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index 7f63ec05bdf4..316b0c3fe745 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -150,6 +150,7 @@ struct tmio_mmc_host {
>         struct mutex            ios_lock;       /* protect set_ios() context */
>         bool                    native_hotplug;
>         bool                    sdio_irq_enabled;
> +       u32                     scc_tappos;
>
>         int (*write16_hook)(struct tmio_mmc_host *host, int addr);
>         int (*clk_enable)(struct tmio_mmc_host *host);
> @@ -160,6 +161,12 @@ struct tmio_mmc_host {
>                               unsigned int direction, int blk_size);
>         int (*start_signal_voltage_switch)(struct mmc_host *mmc,
>                                            struct mmc_ios *ios);
> +       unsigned int (*init_tuning)(struct tmio_mmc_host *host);
> +       void (*prepare_tuning)(struct tmio_mmc_host *host, unsigned long tap);
> +       int (*select_tuning)(struct tmio_mmc_host *host, bool *tap,
> +                            int tap_size);
> +       bool (*retuning)(struct tmio_mmc_host *host);
> +       void (*hw_reset)(struct tmio_mmc_host *host);

Please add the HW reset support in separate patch. I guess you need it
to go in before the tuning support.

>  };
>
>  struct tmio_mmc_host *tmio_mmc_host_alloc(struct platform_device *pdev);
> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
> index a9d07b5f3c63..83b5148a2684 100644
> --- a/drivers/mmc/host/tmio_mmc_pio.c
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -36,6 +36,7 @@
>  #include <linux/io.h>
>  #include <linux/irq.h>
>  #include <linux/mfd/tmio.h>
> +#include <linux/mmc/card.h>
>  #include <linux/mmc/host.h>
>  #include <linux/mmc/mmc.h>
>  #include <linux/mmc/slot-gpio.h>
> @@ -298,6 +299,15 @@ static void tmio_mmc_finish_request(struct tmio_mmc_host *host)
>         if (mrq->cmd->error || (mrq->data && mrq->data->error))
>                 tmio_mmc_abort_dma(host);
>
> +       if (host->retuning) {
> +               int result = host->retuning(host);

This looks like you need to re-tune between each an every request. :-)

Although I guess what really are doing here is that you check if the
auto-retuning has failed, correct?

Perhaps one could update the naming of the new tmio callbacks for
tuning as to make those more self-explained.

> +
> +               if (result || (mrq->cmd->error == -EILSEQ)) {
> +                       mmc_retune_timer_stop(host->mmc);
> +                       mmc_retune_needed(host->mmc);

The mmc core already deals with re-tuning when it get an -EILSEQ error
from a request, so you shouldn't need to manage that here as well.

> +               }
> +       }
> +
>         mmc_request_done(host->mmc, mrq);
>  }
>
> @@ -756,6 +766,68 @@ static int tmio_mmc_start_data(struct tmio_mmc_host *host,
>         return 0;
>  }
>
> +static void tmio_mmc_hw_reset(struct mmc_host *mmc)

As stated earlier, please add the HW reset in a separate patch.

> +{
> +       struct tmio_mmc_host *host = mmc_priv(mmc);
> +
> +       if (host->hw_reset)
> +               host->hw_reset(host);
> +
> +       mmc_retune_timer_stop(host->mmc);
> +       mmc_retune_needed(host->mmc);

This looks like it belongs in the mmc core when it invokes a HW reset
sequence. Please try to move it into there (unless it already covers
this).

> +}
> +
> +static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
> +{
> +       struct tmio_mmc_host *host = mmc_priv(mmc);
> +       unsigned int num;
> +       int i, ret = 0;
> +       bool *tap;
> +
> +       if (!host->init_tuning || !host->select_tuning)

When defining these callbacks, it would be nice to know which ones are
optional and which ones are required.

> +               /* Tuning is not supported */
> +               goto out;
> +
> +       num = host->init_tuning(host);
> +       if (!num)
> +               /* Tuning is not supported */
> +               goto out;
> +
> +       tap = kmalloc(num * 2 * sizeof(*tap), GFP_KERNEL);
> +       if (!tap) {
> +               ret = -ENOMEM;
> +               goto out;
> +       }
> +
> +       /* Issue CMD19 twice for each tap */
> +       for (i = 0; i < 2 * num; i++) {
> +               if (host->prepare_tuning)
> +                       host->prepare_tuning(host, i % num);
> +
> +               ret = mmc_send_tuning(mmc, opcode, NULL);
> +               if (ret && ret != -EILSEQ)
> +                       goto err_free;
> +               tap[i] = (ret != 0);
> +
> +               mdelay(1);
> +       }
> +
> +       ret = host->select_tuning(host, tap, num * 2);
> +
> +err_free:
> +       kfree(tap);
> +out:
> +       if (ret < 0) {
> +               dev_warn(&host->pdev->dev, "Tuning procedure failed\n");
> +               tmio_mmc_hw_reset(mmc);
> +       } else {
> +               host->mmc->retune_period = 0;
> +       }
> +
> +       return ret;
> +
> +}
> +
>  /* Process requests from the MMC layer */
>  static void tmio_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  {
> @@ -978,6 +1050,8 @@ static struct mmc_host_ops tmio_mmc_ops = {
>         .enable_sdio_irq = tmio_mmc_enable_sdio_irq,
>         .card_busy      = tmio_mmc_card_busy,
>         .multi_io_quirk = tmio_multi_io_quirk,
> +       .execute_tuning = tmio_mmc_execute_tuning,
> +       .hw_reset       = tmio_mmc_hw_reset,
>  };
>
>  static int tmio_mmc_init_ocr(struct tmio_mmc_host *host)
> @@ -1202,6 +1276,9 @@ int tmio_mmc_host_runtime_suspend(struct device *dev)
>         struct mmc_host *mmc = dev_get_drvdata(dev);
>         struct tmio_mmc_host *host = mmc_priv(mmc);
>
> +       mmc_retune_timer_stop(host->mmc);
> +       mmc_retune_needed(host->mmc);

I am wondering whether it would it be possible to keep a cache of the
currently used tuning values and then restore these values at
runtime_resume?

In that way you wouldn't need to force new re-tuning cycle here.

> +
>         tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL);
>
>         if (host->clk_cache)
> --
> 2.7.0.rc3.207.g0ac5344
>

Kind regards
Uffe

Re: [PATCH v4 2/4] mmc: tmio: Add tuning support

[Simon Horman]

Hi Ulf,

thanks for your review.
I have tried to address it as best I can below.

On Mon, Aug 22, 2016 at 03:39:03PM +0200, Ulf Hansson wrote:
> On 27 July 2016 at 06:13, Simon Horman <horms+renesas@verge.net.au> wrote:
> > From: Ai Kyuse <ai.kyuse.uw@renesas.com>
> >
> > Add tuning support for use with SDR104 mode
> >
> > Signed-off-by: Ai Kyuse <ai.kyuse.uw@renesas.com>
> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> > ---
> > v4 [Simon Horman]
> > As suggested by Wolfram Sang:
> >   - Do not perform tuning if host->select_tuning is not set:
> >     it seems to make little sense to do so and moreover there is currently
> >     no such use-case
> >   - Do not add mrc->sbc handling from tmio_mmc_request,
> >     this is a hang-over from earlier versions of this patchset which
> >     did not use core infrastructure for retuning
> >   - Tidy up local variable usage
> > * Correct index passed to prepare_tuning(): this seems to have
> >   been the last piece of resolving the timeouts during tuning puzzle
> > * Further cleanups to tmio_mmc_execute_tuning():
> >   - Ensure tap is sized proportionally to its members
> >   - Remove stray '*' in comment
> >   - Use mmc rather than host->mmc, these are equivalent but
> >     the former seems tidier
> >   - Correct inverted logic in setting tap values
> > * Re-introduce retuning support. This was removed in v3.
> >
> > v3 [Simon Horman]
> > * As suggested by Kuninori Morimoto:
> >   - Do not add unused retuning callback to struct tmio_mmc_host
> >   - Change return type of prepare_tuning callback to void
> >   - Add tap_size parameter to select_tuning callback
> >
> > v2 [Simon Horman]
> > * As suggested by Kuninori Morimoto:
> >   - Actually remove unnecessary TMIO_MMC_HAS_UHS_SCC define
> > * As suggested by Wolfram Sang:
> >   - Rely on core to call tuning. This simplifies things somewhat.
> >   - Use mmc_send_tuning()
> >     - A side affect of this appears to be that we now see some recoverable
> >       errors logged during tuning. These are typically corrected by
> >       subsequent tuning. It is the logging that is the apparent side effect
> >       of this change.
> >       e.g.
> >       sh_mobile_sdhi ee100000.sd: timeout waiting for hardware interrupt (CMD19)
> >       sh_mobile_sdhi ee100000.sd: Tuning procedure failed
> > * Use bool rather than unsigned long to pass test status
> >   to select_tuning() callback
> > * Do not retune if init_tuning callback is not present or
> >   indicates that there are no taps present
> > * Retune on hardware reset
> >
> > v1 [Simon Horman]
> > * Omit start_signal_voltage_switch and tmio_mmc_card_busy changes which are
> >   already present in mainline in a different form
> > * Return num from init_tuning rather than passing an extra parameter
> >   to hold the return value
> > * Only call host->init_tuning if it is non-NULL
> > * Place tmio_mmc_execute_tuning() such that no new forward declarations are
> >   required
> > * Remove unused TMIO_MMC_HAS_UHS_SCC define
> >
> > v0 [Ai Kyuse]
> >
> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> > ---
> >  drivers/mmc/host/tmio_mmc.h     |  7 ++++
> >  drivers/mmc/host/tmio_mmc_pio.c | 77 +++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 84 insertions(+)
> >
> > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> > index 7f63ec05bdf4..316b0c3fe745 100644
> > --- a/drivers/mmc/host/tmio_mmc.h
> > +++ b/drivers/mmc/host/tmio_mmc.h
> > @@ -150,6 +150,7 @@ struct tmio_mmc_host {
> >         struct mutex            ios_lock;       /* protect set_ios() context */
> >         bool                    native_hotplug;
> >         bool                    sdio_irq_enabled;
> > +       u32                     scc_tappos;
> >
> >         int (*write16_hook)(struct tmio_mmc_host *host, int addr);
> >         int (*clk_enable)(struct tmio_mmc_host *host);
> > @@ -160,6 +161,12 @@ struct tmio_mmc_host {
> >                               unsigned int direction, int blk_size);
> >         int (*start_signal_voltage_switch)(struct mmc_host *mmc,
> >                                            struct mmc_ios *ios);
> > +       unsigned int (*init_tuning)(struct tmio_mmc_host *host);
> > +       void (*prepare_tuning)(struct tmio_mmc_host *host, unsigned long tap);
> > +       int (*select_tuning)(struct tmio_mmc_host *host, bool *tap,
> > +                            int tap_size);
> > +       bool (*retuning)(struct tmio_mmc_host *host);
> > +       void (*hw_reset)(struct tmio_mmc_host *host);
> 
> Please add the HW reset support in separate patch. I guess you need it
> to go in before the tuning support.

Sure, will do.
And yes, I think it needs go go in before tuning support.

> >  };
> >
> >  struct tmio_mmc_host *tmio_mmc_host_alloc(struct platform_device *pdev);
> > diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
> > index a9d07b5f3c63..83b5148a2684 100644
> > --- a/drivers/mmc/host/tmio_mmc_pio.c
> > +++ b/drivers/mmc/host/tmio_mmc_pio.c
> > @@ -36,6 +36,7 @@
> >  #include <linux/io.h>
> >  #include <linux/irq.h>
> >  #include <linux/mfd/tmio.h>
> > +#include <linux/mmc/card.h>
> >  #include <linux/mmc/host.h>
> >  #include <linux/mmc/mmc.h>
> >  #include <linux/mmc/slot-gpio.h>
> > @@ -298,6 +299,15 @@ static void tmio_mmc_finish_request(struct tmio_mmc_host *host)
> >         if (mrq->cmd->error || (mrq->data && mrq->data->error))
> >                 tmio_mmc_abort_dma(host);
> >
> > +       if (host->retuning) {
> > +               int result = host->retuning(host);
> 
> This looks like you need to re-tune between each an every request. :-)
> 
> Although I guess what really are doing here is that you check if the
> auto-retuning has failed, correct?
> 
> Perhaps one could update the naming of the new tmio callbacks for
> tuning as to make those more self-explained.

Perhaps calling it check_scc_error would be better,
that is what the callback actually does

> 
> > +
> > +               if (result || (mrq->cmd->error == -EILSEQ)) {
> > +                       mmc_retune_timer_stop(host->mmc);
> > +                       mmc_retune_needed(host->mmc);
> 
> The mmc core already deals with re-tuning when it get an -EILSEQ error
> from a request, so you shouldn't need to manage that here as well.

Thanks, I'll clean that up.

> 
> > +               }
> > +       }
> > +
> >         mmc_request_done(host->mmc, mrq);
> >  }
> >
> > @@ -756,6 +766,68 @@ static int tmio_mmc_start_data(struct tmio_mmc_host *host,
> >         return 0;
> >  }
> >
> > +static void tmio_mmc_hw_reset(struct mmc_host *mmc)
> 
> As stated earlier, please add the HW reset in a separate patch.
> 
> > +{
> > +       struct tmio_mmc_host *host = mmc_priv(mmc);
> > +
> > +       if (host->hw_reset)
> > +               host->hw_reset(host);
> > +
> > +       mmc_retune_timer_stop(host->mmc);
> > +       mmc_retune_needed(host->mmc);
> 
> This looks like it belongs in the mmc core when it invokes a HW reset
> sequence. Please try to move it into there (unless it already covers
> this).

I think it is already handled by the core and I propose updating this patch as
follows:

@@ -772,9 +772,6 @@ static void tmio_mmc_hw_reset(struct mmc_host *mmc)
 
 	if (host->hw_reset)
 		host->hw_reset(host);
-
-	mmc_retune_timer_stop(host->mmc);
-	mmc_retune_needed(host->mmc);
 }
 
 static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
@@ -819,7 +816,7 @@ err_free:
 out:
 	if (ret < 0) {
 		dev_warn(&host->pdev->dev, "Tuning procedure failed\n");
-		tmio_mmc_hw_reset(mmc);
+		mmc_hw_reset(mmc);
 	} else {
 		host->mmc->retune_period = 0;
 	}

> > +}
> > +
> > +static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
> > +{
> > +       struct tmio_mmc_host *host = mmc_priv(mmc);
> > +       unsigned int num;
> > +       int i, ret = 0;
> > +       bool *tap;
> > +
> > +       if (!host->init_tuning || !host->select_tuning)
> 
> When defining these callbacks, it would be nice to know which ones are
> optional and which ones are required.

Would some comments in struct tmio_mmc_host be an appropriate way
to achieve that?

> > +               /* Tuning is not supported */
> > +               goto out;
> > +
> > +       num = host->init_tuning(host);
> > +       if (!num)
> > +               /* Tuning is not supported */
> > +               goto out;
> > +
> > +       tap = kmalloc(num * 2 * sizeof(*tap), GFP_KERNEL);
> > +       if (!tap) {
> > +               ret = -ENOMEM;
> > +               goto out;
> > +       }
> > +
> > +       /* Issue CMD19 twice for each tap */
> > +       for (i = 0; i < 2 * num; i++) {
> > +               if (host->prepare_tuning)
> > +                       host->prepare_tuning(host, i % num);
> > +
> > +               ret = mmc_send_tuning(mmc, opcode, NULL);
> > +               if (ret && ret != -EILSEQ)
> > +                       goto err_free;
> > +               tap[i] = (ret != 0);
> > +
> > +               mdelay(1);
> > +       }
> > +
> > +       ret = host->select_tuning(host, tap, num * 2);
> > +
> > +err_free:
> > +       kfree(tap);
> > +out:
> > +       if (ret < 0) {
> > +               dev_warn(&host->pdev->dev, "Tuning procedure failed\n");
> > +               tmio_mmc_hw_reset(mmc);
> > +       } else {
> > +               host->mmc->retune_period = 0;
> > +       }
> > +
> > +       return ret;
> > +
> > +}
> > +
> >  /* Process requests from the MMC layer */
> >  static void tmio_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
> >  {
> > @@ -978,6 +1050,8 @@ static struct mmc_host_ops tmio_mmc_ops = {
> >         .enable_sdio_irq = tmio_mmc_enable_sdio_irq,
> >         .card_busy      = tmio_mmc_card_busy,
> >         .multi_io_quirk = tmio_multi_io_quirk,
> > +       .execute_tuning = tmio_mmc_execute_tuning,
> > +       .hw_reset       = tmio_mmc_hw_reset,
> >  };
> >
> >  static int tmio_mmc_init_ocr(struct tmio_mmc_host *host)
> > @@ -1202,6 +1276,9 @@ int tmio_mmc_host_runtime_suspend(struct device *dev)
> >         struct mmc_host *mmc = dev_get_drvdata(dev);
> >         struct tmio_mmc_host *host = mmc_priv(mmc);
> >
> > +       mmc_retune_timer_stop(host->mmc);
> > +       mmc_retune_needed(host->mmc);
> 
> I am wondering whether it would it be possible to keep a cache of the
> currently used tuning values and then restore these values at
> runtime_resume?
> 
> In that way you wouldn't need to force new re-tuning cycle here.

I will check with the hardware people to see if it is practical from
that POV.

>From a software POV that ought to be simple enough: a small bitmap ought
to be sufficient to save the values for the hardware covered by this
series.

> > +
> >         tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL);
> >
> >         if (host->clk_cache)
> > --
> > 2.7.0.rc3.207.g0ac5344

Re: [PATCH v4 2/4] mmc: tmio: Add tuning support

[Ulf Hansson]

[...]

>> > +static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> > +{
>> > +       struct tmio_mmc_host *host = mmc_priv(mmc);
>> > +       unsigned int num;
>> > +       int i, ret = 0;
>> > +       bool *tap;
>> > +
>> > +       if (!host->init_tuning || !host->select_tuning)
>>
>> When defining these callbacks, it would be nice to know which ones are
>> optional and which ones are required.
>
> Would some comments in struct tmio_mmc_host be an appropriate way
> to achieve that?

Yes, that would be nice!

[...]

>> >  static int tmio_mmc_init_ocr(struct tmio_mmc_host *host)
>> > @@ -1202,6 +1276,9 @@ int tmio_mmc_host_runtime_suspend(struct device *dev)
>> >         struct mmc_host *mmc = dev_get_drvdata(dev);
>> >         struct tmio_mmc_host *host = mmc_priv(mmc);
>> >
>> > +       mmc_retune_timer_stop(host->mmc);
>> > +       mmc_retune_needed(host->mmc);
>>
>> I am wondering whether it would it be possible to keep a cache of the
>> currently used tuning values and then restore these values at
>> runtime_resume?
>>
>> In that way you wouldn't need to force new re-tuning cycle here.
>
> I will check with the hardware people to see if it is practical from
> that POV.

Okay!

>
> From a software POV that ought to be simple enough: a small bitmap ought
> to be sufficient to save the values for the hardware covered by this
> series.

Yes, indeed!

Kind regards
Uffe

Re: [PATCH v4 2/4] mmc: tmio: Add tuning support

[Simon Horman]

On Tue, Aug 23, 2016 at 05:02:56PM +0200, Ulf Hansson wrote:
> [...]
> 
> >> > +static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
> >> > +{
> >> > +       struct tmio_mmc_host *host = mmc_priv(mmc);
> >> > +       unsigned int num;
> >> > +       int i, ret = 0;
> >> > +       bool *tap;
> >> > +
> >> > +       if (!host->init_tuning || !host->select_tuning)
> >>
> >> When defining these callbacks, it would be nice to know which ones are
> >> optional and which ones are required.
> >
> > Would some comments in struct tmio_mmc_host be an appropriate way
> > to achieve that?
> 
> Yes, that would be nice!
> 
> [...]
> 
> >> >  static int tmio_mmc_init_ocr(struct tmio_mmc_host *host)
> >> > @@ -1202,6 +1276,9 @@ int tmio_mmc_host_runtime_suspend(struct device *dev)
> >> >         struct mmc_host *mmc = dev_get_drvdata(dev);
> >> >         struct tmio_mmc_host *host = mmc_priv(mmc);
> >> >
> >> > +       mmc_retune_timer_stop(host->mmc);
> >> > +       mmc_retune_needed(host->mmc);
> >>
> >> I am wondering whether it would it be possible to keep a cache of the
> >> currently used tuning values and then restore these values at
> >> runtime_resume?
> >>
> >> In that way you wouldn't need to force new re-tuning cycle here.
> >
> > I will check with the hardware people to see if it is practical from
> > that POV.
> 
> Okay!

The feedback I received is something like this:

* The tap values calculated during retuning depend on the temperature of
  the SoC and card.
* So after resume the values may be invalid.
* It may be possible to re-use the TAP values and re-tune if a transfer
  fails but the people I spoke with were unsure of the merit of that
  approach

Personally my feeling is that we should initiate a retune on resume for
now as that seems to be the safest option.

> > From a software POV that ought to be simple enough: a small bitmap ought
> > to be sufficient to save the values for the hardware covered by this
> > series.
> 
> Yes, indeed!
> 
> Kind regards
> Uffe
> 

Re: [PATCH v4 2/4] mmc: tmio: Add tuning support

[Ulf Hansson]

On 25 August 2016 at 14:04, Simon Horman <horms@verge.net.au> wrote:
> On Tue, Aug 23, 2016 at 05:02:56PM +0200, Ulf Hansson wrote:
>> [...]
>>
>> >> > +static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> >> > +{
>> >> > +       struct tmio_mmc_host *host = mmc_priv(mmc);
>> >> > +       unsigned int num;
>> >> > +       int i, ret = 0;
>> >> > +       bool *tap;
>> >> > +
>> >> > +       if (!host->init_tuning || !host->select_tuning)
>> >>
>> >> When defining these callbacks, it would be nice to know which ones are
>> >> optional and which ones are required.
>> >
>> > Would some comments in struct tmio_mmc_host be an appropriate way
>> > to achieve that?
>>
>> Yes, that would be nice!
>>
>> [...]
>>
>> >> >  static int tmio_mmc_init_ocr(struct tmio_mmc_host *host)
>> >> > @@ -1202,6 +1276,9 @@ int tmio_mmc_host_runtime_suspend(struct device *dev)
>> >> >         struct mmc_host *mmc = dev_get_drvdata(dev);
>> >> >         struct tmio_mmc_host *host = mmc_priv(mmc);
>> >> >
>> >> > +       mmc_retune_timer_stop(host->mmc);
>> >> > +       mmc_retune_needed(host->mmc);
>> >>
>> >> I am wondering whether it would it be possible to keep a cache of the
>> >> currently used tuning values and then restore these values at
>> >> runtime_resume?
>> >>
>> >> In that way you wouldn't need to force new re-tuning cycle here.
>> >
>> > I will check with the hardware people to see if it is practical from
>> > that POV.
>>
>> Okay!
>
> The feedback I received is something like this:
>
> * The tap values calculated during retuning depend on the temperature of
>   the SoC and card.
> * So after resume the values may be invalid.

They values may also become invalid during long continues transfers,
which prevents the ->runtime_suspend() callback to be invoked -
because the temperature may increase.

> * It may be possible to re-use the TAP values and re-tune if a transfer
>   fails but the people I spoke with were unsure of the merit of that
>   approach

There's is a huge benefit!

You will get a significant decreased latency at ->runtime_resume() as
you don't need to run a complete re-tuning cycle.

I can't really give you fresh numbers as it's a long time I tried this
myself, although if you did some measurements on this it would be
nice! :-)

>
> Personally my feeling is that we should initiate a retune on resume for
> now as that seems to be the safest option.

I don't agree. :-) I think it's better to postpone re-tune until it's
actually needed.

Re-tune happens in the request error handling path, but also if you
configure a re-tuning period. That ought to be sufficient, don't you
think?

[...]

Kind regards
Uffe

Re: [PATCH v4 2/4] mmc: tmio: Add tuning support

[Simon Horman]

On Fri, Aug 26, 2016 at 10:01:35AM +0200, Ulf Hansson wrote:
> On 25 August 2016 at 14:04, Simon Horman <horms@verge.net.au> wrote:
> > On Tue, Aug 23, 2016 at 05:02:56PM +0200, Ulf Hansson wrote:

...

> >> >> I am wondering whether it would it be possible to keep a cache of the
> >> >> currently used tuning values and then restore these values at
> >> >> runtime_resume?
> >> >>
> >> >> In that way you wouldn't need to force new re-tuning cycle here.
> >> >
> >> > I will check with the hardware people to see if it is practical from
> >> > that POV.
> >>
> >> Okay!
> >
> > The feedback I received is something like this:
> >
> > * The tap values calculated during retuning depend on the temperature of
> >   the SoC and card.
> > * So after resume the values may be invalid.
> 
> They values may also become invalid during long continues transfers,
> which prevents the ->runtime_suspend() callback to be invoked -
> because the temperature may increase.
> 
> > * It may be possible to re-use the TAP values and re-tune if a transfer
> >   fails but the people I spoke with were unsure of the merit of that
> >   approach
> 
> There's is a huge benefit!
> 
> You will get a significant decreased latency at ->runtime_resume() as
> you don't need to run a complete re-tuning cycle.
> 
> I can't really give you fresh numbers as it's a long time I tried this
> myself, although if you did some measurements on this it would be
> nice! :-)
> 
> >
> > Personally my feeling is that we should initiate a retune on resume for
> > now as that seems to be the safest option.
> 
> I don't agree. :-) I think it's better to postpone re-tune until it's
> actually needed.
> 
> Re-tune happens in the request error handling path, but also if you
> configure a re-tuning period. That ought to be sufficient, don't you
> think?

Hi Ulf,

Is something like this close to what you have in mind?

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 94e79449c533..fc43cf5ce958 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -357,11 +357,9 @@ static void sh_mobile_sdhi_prepare_tuning(struct tmio_mmc_host *host,
 
 #define SH_MOBILE_SDHI_MAX_TAP 3
 
-static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host,
-					bool *tap, int tap_size)
+static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host)
 {
 	struct sh_mobile_sdhi *priv = host_to_priv(host);
-	unsigned long tap_num;  /* total number of taps */
 	unsigned long tap_cnt;  /* counter of tuning success */
 	unsigned long tap_set;  /* tap position */
 	unsigned long tap_start;/* start position of tuning success */
@@ -372,14 +370,6 @@ static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host,
 	/* Clear SCC_RVSREQ */
 	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSREQ, 0);
 
-	/* Select SCC */
-	tap_num = (sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_DTCNTL) >>
-		   SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_SHIFT) &
-		SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_MASK;
-
-	if (tap_num * 2 != tap_size)
-		return -EINVAL;
-
 	/*
 	 * Find the longest consecutive run of successful probes.  If that
 	 * is more than SH_MOBILE_SDHI_MAX_TAP probes long then use the
@@ -389,8 +379,8 @@ static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host,
 	ntap = 0;
 	tap_start = 0;
 	tap_end = 0;
-	for (i = 0; i < tap_num * 2; i++) {
-		if (tap[i] == 0)
+	for (i = 0; i < host->tap_num * 2; i++) {
+		if (test_bit(i, host->taps))
 			ntap++;
 		else {
 			if (ntap > tap_cnt) {
@@ -409,7 +399,7 @@ static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host,
 	}
 
 	if (tap_cnt >= SH_MOBILE_SDHI_MAX_TAP)
-		tap_set = (tap_start + tap_end) / 2 % tap_num;
+		tap_set = (tap_start + tap_end) / 2 % host->tap_num;
 	else
 		return -EIO;
 
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index c932fe876690..4186045cce0d 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -171,8 +171,12 @@ struct tmio_mmc_host {
 	/* Mandatory callback for tuning to occur which is
 	 * optional for SDR50 and mandatory for SDR104 */
 	unsigned int (*init_tuning)(struct tmio_mmc_host *host);
-	int (*select_tuning)(struct tmio_mmc_host *host, bool *tap,
-			     int tap_size);
+	int (*select_tuning)(struct tmio_mmc_host *host);
+
+	/* Tuning values: 1 for success, 0 for failure */
+	DECLARE_BITMAP(taps, BITS_PER_BYTE * sizeof(long));
+	unsigned int tap_num;
+	bool use_saved_taps;
 };
 
 struct tmio_mmc_host *tmio_mmc_host_alloc(struct platform_device *pdev);
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index ded8fa53e0a0..2b5c8b090ed3 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -773,43 +773,50 @@ static void tmio_mmc_hw_reset(struct mmc_host *mmc)
 static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
 {
 	struct tmio_mmc_host *host = mmc_priv(mmc);
-	unsigned int num;
-	int i, ret = 0;
-	bool *tap;
+	int ret = 0;
 
-	if (!host->init_tuning || !host->select_tuning)
-		/* Tuning is not supported */
-		goto out;
+	if (!host->tap_num) {
+		if (!host->init_tuning || !host->select_tuning)
+			/* Tuning is not supported */
+			goto out;
 
-	num = host->init_tuning(host);
-	if (!num)
-		/* Tuning is not supported */
-		goto out;
+		host->tap_num = host->init_tuning(host);
+		if (!host->tap_num)
+			/* Tuning is not supported */
+			goto out;
 
-	tap = kmalloc(num * 2 * sizeof(*tap), GFP_KERNEL);
-	if (!tap) {
-		ret = -ENOMEM;
-		goto out;
+		if (host->tap_num * 2 >= sizeof(host->taps) * BITS_PER_BYTE)
+			dev_warn_once(&host->pdev->dev,
+			      "Too many taps, skipping tuning. Please consider "
+			      "updating size of taps field of tmio_mmc_host\n");
+
+		host->use_saved_taps = false;
 	}
 
-	/* Issue CMD19 twice for each tap */
-	for (i = 0; i < 2 * num; i++) {
-		if (host->prepare_tuning)
-			host->prepare_tuning(host, i % num);
+	if (!host->use_saved_taps) {
+		int i;
+
+		bitmap_zero(host->taps, host->tap_num * 2);
 
-		ret = mmc_send_tuning(mmc, opcode, NULL);
-		if (ret && ret != -EILSEQ)
-			goto err_free;
-		tap[i] = (ret != 0);
+		/* Issue CMD19 twice for each tap */
+		for (i = 0; i < 2 * host->tap_num; i++) {
+			if (host->prepare_tuning)
+				host->prepare_tuning(host, i % host->tap_num);
 
-		mdelay(1);
+			ret = mmc_send_tuning(mmc, opcode, NULL);
+			if (ret && ret != -EILSEQ)
+				goto out;
+			if (ret == 0)
+				set_bit(i, host->taps);
+
+			mdelay(1);
+		}
 	}
 
-	ret = host->select_tuning(host, tap, num * 2);
+	ret = host->select_tuning(host);
 
-err_free:
-	kfree(tap);
 out:
+	host->use_saved_taps = false;
 	if (ret < 0) {
 		dev_warn(&host->pdev->dev, "Tuning procedure failed\n");
 		mmc_hw_reset(mmc);
@@ -1271,6 +1278,7 @@ int tmio_mmc_host_runtime_suspend(struct device *dev)
 
 	mmc_retune_timer_stop(host->mmc);
 	mmc_retune_needed(host->mmc);
+	host->use_saved_taps = true;
 
 	tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL);
 

Re: [PATCH v4 2/4] mmc: tmio: Add tuning support

[Ulf Hansson]

On 29 August 2016 at 14:05, Simon Horman <horms@verge.net.au> wrote:
> On Fri, Aug 26, 2016 at 10:01:35AM +0200, Ulf Hansson wrote:
>> On 25 August 2016 at 14:04, Simon Horman <horms@verge.net.au> wrote:
>> > On Tue, Aug 23, 2016 at 05:02:56PM +0200, Ulf Hansson wrote:
>
> ...
>
>> >> >> I am wondering whether it would it be possible to keep a cache of the
>> >> >> currently used tuning values and then restore these values at
>> >> >> runtime_resume?
>> >> >>
>> >> >> In that way you wouldn't need to force new re-tuning cycle here.
>> >> >
>> >> > I will check with the hardware people to see if it is practical from
>> >> > that POV.
>> >>
>> >> Okay!
>> >
>> > The feedback I received is something like this:
>> >
>> > * The tap values calculated during retuning depend on the temperature of
>> >   the SoC and card.
>> > * So after resume the values may be invalid.
>>
>> They values may also become invalid during long continues transfers,
>> which prevents the ->runtime_suspend() callback to be invoked -
>> because the temperature may increase.
>>
>> > * It may be possible to re-use the TAP values and re-tune if a transfer
>> >   fails but the people I spoke with were unsure of the merit of that
>> >   approach
>>
>> There's is a huge benefit!
>>
>> You will get a significant decreased latency at ->runtime_resume() as
>> you don't need to run a complete re-tuning cycle.
>>
>> I can't really give you fresh numbers as it's a long time I tried this
>> myself, although if you did some measurements on this it would be
>> nice! :-)
>>
>> >
>> > Personally my feeling is that we should initiate a retune on resume for
>> > now as that seems to be the safest option.
>>
>> I don't agree. :-) I think it's better to postpone re-tune until it's
>> actually needed.
>>
>> Re-tune happens in the request error handling path, but also if you
>> configure a re-tuning period. That ought to be sufficient, don't you
>> think?
>
> Hi Ulf,
>
> Is something like this close to what you have in mind?
>
> diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
> index 94e79449c533..fc43cf5ce958 100644
> --- a/drivers/mmc/host/sh_mobile_sdhi.c
> +++ b/drivers/mmc/host/sh_mobile_sdhi.c
> @@ -357,11 +357,9 @@ static void sh_mobile_sdhi_prepare_tuning(struct tmio_mmc_host *host,
>
>  #define SH_MOBILE_SDHI_MAX_TAP 3
>
> -static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host,
> -                                       bool *tap, int tap_size)
> +static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host)
>  {
>         struct sh_mobile_sdhi *priv = host_to_priv(host);
> -       unsigned long tap_num;  /* total number of taps */
>         unsigned long tap_cnt;  /* counter of tuning success */
>         unsigned long tap_set;  /* tap position */
>         unsigned long tap_start;/* start position of tuning success */
> @@ -372,14 +370,6 @@ static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host,
>         /* Clear SCC_RVSREQ */
>         sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSREQ, 0);
>
> -       /* Select SCC */
> -       tap_num = (sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_DTCNTL) >>
> -                  SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_SHIFT) &
> -               SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_MASK;
> -
> -       if (tap_num * 2 != tap_size)
> -               return -EINVAL;
> -
>         /*
>          * Find the longest consecutive run of successful probes.  If that
>          * is more than SH_MOBILE_SDHI_MAX_TAP probes long then use the
> @@ -389,8 +379,8 @@ static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host,
>         ntap = 0;
>         tap_start = 0;
>         tap_end = 0;
> -       for (i = 0; i < tap_num * 2; i++) {
> -               if (tap[i] == 0)
> +       for (i = 0; i < host->tap_num * 2; i++) {
> +               if (test_bit(i, host->taps))
>                         ntap++;
>                 else {
>                         if (ntap > tap_cnt) {
> @@ -409,7 +399,7 @@ static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host,
>         }
>
>         if (tap_cnt >= SH_MOBILE_SDHI_MAX_TAP)
> -               tap_set = (tap_start + tap_end) / 2 % tap_num;
> +               tap_set = (tap_start + tap_end) / 2 % host->tap_num;
>         else
>                 return -EIO;
>
> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index c932fe876690..4186045cce0d 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -171,8 +171,12 @@ struct tmio_mmc_host {
>         /* Mandatory callback for tuning to occur which is
>          * optional for SDR50 and mandatory for SDR104 */
>         unsigned int (*init_tuning)(struct tmio_mmc_host *host);
> -       int (*select_tuning)(struct tmio_mmc_host *host, bool *tap,
> -                            int tap_size);
> +       int (*select_tuning)(struct tmio_mmc_host *host);
> +
> +       /* Tuning values: 1 for success, 0 for failure */
> +       DECLARE_BITMAP(taps, BITS_PER_BYTE * sizeof(long));
> +       unsigned int tap_num;
> +       bool use_saved_taps;
>  };
>
>  struct tmio_mmc_host *tmio_mmc_host_alloc(struct platform_device *pdev);
> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
> index ded8fa53e0a0..2b5c8b090ed3 100644
> --- a/drivers/mmc/host/tmio_mmc_pio.c
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -773,43 +773,50 @@ static void tmio_mmc_hw_reset(struct mmc_host *mmc)
>  static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
>  {
>         struct tmio_mmc_host *host = mmc_priv(mmc);
> -       unsigned int num;
> -       int i, ret = 0;
> -       bool *tap;
> +       int ret = 0;
>
> -       if (!host->init_tuning || !host->select_tuning)
> -               /* Tuning is not supported */
> -               goto out;
> +       if (!host->tap_num) {
> +               if (!host->init_tuning || !host->select_tuning)
> +                       /* Tuning is not supported */
> +                       goto out;
>
> -       num = host->init_tuning(host);
> -       if (!num)
> -               /* Tuning is not supported */
> -               goto out;
> +               host->tap_num = host->init_tuning(host);
> +               if (!host->tap_num)
> +                       /* Tuning is not supported */
> +                       goto out;
>
> -       tap = kmalloc(num * 2 * sizeof(*tap), GFP_KERNEL);
> -       if (!tap) {
> -               ret = -ENOMEM;
> -               goto out;
> +               if (host->tap_num * 2 >= sizeof(host->taps) * BITS_PER_BYTE)
> +                       dev_warn_once(&host->pdev->dev,
> +                             "Too many taps, skipping tuning. Please consider "
> +                             "updating size of taps field of tmio_mmc_host\n");
> +
> +               host->use_saved_taps = false;
>         }
>
> -       /* Issue CMD19 twice for each tap */
> -       for (i = 0; i < 2 * num; i++) {
> -               if (host->prepare_tuning)
> -                       host->prepare_tuning(host, i % num);
> +       if (!host->use_saved_taps) {
> +               int i;
> +
> +               bitmap_zero(host->taps, host->tap_num * 2);
>
> -               ret = mmc_send_tuning(mmc, opcode, NULL);
> -               if (ret && ret != -EILSEQ)
> -                       goto err_free;
> -               tap[i] = (ret != 0);
> +               /* Issue CMD19 twice for each tap */
> +               for (i = 0; i < 2 * host->tap_num; i++) {
> +                       if (host->prepare_tuning)
> +                               host->prepare_tuning(host, i % host->tap_num);
>
> -               mdelay(1);
> +                       ret = mmc_send_tuning(mmc, opcode, NULL);
> +                       if (ret && ret != -EILSEQ)
> +                               goto out;
> +                       if (ret == 0)
> +                               set_bit(i, host->taps);
> +
> +                       mdelay(1);
> +               }
>         }
>
> -       ret = host->select_tuning(host, tap, num * 2);
> +       ret = host->select_tuning(host);
>
> -err_free:
> -       kfree(tap);
>  out:
> +       host->use_saved_taps = false;
>         if (ret < 0) {
>                 dev_warn(&host->pdev->dev, "Tuning procedure failed\n");
>                 mmc_hw_reset(mmc);
> @@ -1271,6 +1278,7 @@ int tmio_mmc_host_runtime_suspend(struct device *dev)
>
>         mmc_retune_timer_stop(host->mmc);
>         mmc_retune_needed(host->mmc);
> +       host->use_saved_taps = true;

I don't think you should trigger a re-tune here at all. Moreover you
don't need to keep track of whether you have valid tap values by using
the ->use_saved_taps variable, the mmc core deals with this for you.

Instead, you should restore the tap values by invoking
host->select_tuning() from the ->runtime_resume() callback, although
only when host->mmc->can_retune == 1. We should probably add a helper
function in the mmc core for this check, instead of accessing
"can_retune" directly.

>
>         tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL);
>

Kind regards
Uffe

Re: [PATCH v4 2/4] mmc: tmio: Add tuning support

[Simon Horman]

On Mon, Aug 29, 2016 at 04:05:55PM +0200, Ulf Hansson wrote:
> On 29 August 2016 at 14:05, Simon Horman <horms@verge.net.au> wrote:
> > On Fri, Aug 26, 2016 at 10:01:35AM +0200, Ulf Hansson wrote:
> >> On 25 August 2016 at 14:04, Simon Horman <horms@verge.net.au> wrote:
> >> > On Tue, Aug 23, 2016 at 05:02:56PM +0200, Ulf Hansson wrote:
> >
> > ...

...

> > @@ -1271,6 +1278,7 @@ int tmio_mmc_host_runtime_suspend(struct device *dev)
> >
> >         mmc_retune_timer_stop(host->mmc);
> >         mmc_retune_needed(host->mmc);
> > +       host->use_saved_taps = true;
> 
> I don't think you should trigger a re-tune here at all. Moreover you
> don't need to keep track of whether you have valid tap values by using
> the ->use_saved_taps variable, the mmc core deals with this for you.
> 
> Instead, you should restore the tap values by invoking
> host->select_tuning() from the ->runtime_resume() callback, although
> only when host->mmc->can_retune == 1. We should probably add a helper
> function in the mmc core for this check, instead of accessing
> "can_retune" directly.

Thanks, I was wondering what the best way to handle this is.

I tried your suggestion above but it seems that host->mmc->can_retune is
zero when ->runtime_resume() is called because of the following call paths:

a) _mmc_sd_suspend()
     -> mmc_power_off()
        -> mmc_set_initial_state()
	   -> mmc_retune_disable
   and
b) mmc_sd_runtime_resume()
     -> mmc_power_up
        -> mmc_set_initial_state()
	   -> mmc_retune_disable

> >         tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL);


I plan to repost this patchset without the tap restoration code.
This is because I have a number of changes locally, in particular
to address other parts of your review, and I would like
them to see the light of day.

I will mark the tap restoration as a TODO item which we can address
before merging the code in question.

Re: [PATCH v4 2/4] mmc: tmio: Add tuning support

[Ulf Hansson]

On 30 August 2016 at 22:51, Simon Horman <horms@verge.net.au> wrote:
> On Mon, Aug 29, 2016 at 04:05:55PM +0200, Ulf Hansson wrote:
>> On 29 August 2016 at 14:05, Simon Horman <horms@verge.net.au> wrote:
>> > On Fri, Aug 26, 2016 at 10:01:35AM +0200, Ulf Hansson wrote:
>> >> On 25 August 2016 at 14:04, Simon Horman <horms@verge.net.au> wrote:
>> >> > On Tue, Aug 23, 2016 at 05:02:56PM +0200, Ulf Hansson wrote:
>> >
>> > ...
>
> ...
>
>> > @@ -1271,6 +1278,7 @@ int tmio_mmc_host_runtime_suspend(struct device *dev)
>> >
>> >         mmc_retune_timer_stop(host->mmc);
>> >         mmc_retune_needed(host->mmc);
>> > +       host->use_saved_taps = true;
>>
>> I don't think you should trigger a re-tune here at all. Moreover you
>> don't need to keep track of whether you have valid tap values by using
>> the ->use_saved_taps variable, the mmc core deals with this for you.
>>
>> Instead, you should restore the tap values by invoking
>> host->select_tuning() from the ->runtime_resume() callback, although
>> only when host->mmc->can_retune == 1. We should probably add a helper
>> function in the mmc core for this check, instead of accessing
>> "can_retune" directly.
>
> Thanks, I was wondering what the best way to handle this is.
>
> I tried your suggestion above but it seems that host->mmc->can_retune is
> zero when ->runtime_resume() is called because of the following call paths:
>
> a) _mmc_sd_suspend()
>      -> mmc_power_off()
>         -> mmc_set_initial_state()
>            -> mmc_retune_disable
>    and
> b) mmc_sd_runtime_resume()
>      -> mmc_power_up
>         -> mmc_set_initial_state()
>            -> mmc_retune_disable
>
>> >         tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL);

This is exactly what shall happen :-)

If the mmc core suspends the card, it means it will also re-initialize
the card when resuming it. In that way the regular tuning sequence
will take place as a part of re-initialization of the card, so you
definitely must not restore tap values in these case. That is why you
should be using the can_retune to know when to restore the values.

>
>
> I plan to repost this patchset without the tap restoration code.
> This is because I have a number of changes locally, in particular
> to address other parts of your review, and I would like
> them to see the light of day.
>
> I will mark the tap restoration as a TODO item which we can address
> before merging the code in question.

It shouldn't be that hard to implement this, so I rather see that we
get this right from the beginning.

As matter of fact it probably requires less code than you had in your
initial approach, especially since I don't think you need to deal with
anything tuning related in the ->runtime_suspend() callback, but only
in ->runtime_resume().

Kind regards
Uffe

Re: [PATCH v4 2/4] mmc: tmio: Add tuning support

[Simon Horman]

On Wed, Aug 31, 2016 at 09:38:40AM +0200, Ulf Hansson wrote:
> On 30 August 2016 at 22:51, Simon Horman <horms@verge.net.au> wrote:
> > On Mon, Aug 29, 2016 at 04:05:55PM +0200, Ulf Hansson wrote:
> >> On 29 August 2016 at 14:05, Simon Horman <horms@verge.net.au> wrote:
> >> > On Fri, Aug 26, 2016 at 10:01:35AM +0200, Ulf Hansson wrote:
> >> >> On 25 August 2016 at 14:04, Simon Horman <horms@verge.net.au> wrote:
> >> >> > On Tue, Aug 23, 2016 at 05:02:56PM +0200, Ulf Hansson wrote:
> >> >
> >> > ...
> >
> > ...
> >
> >> > @@ -1271,6 +1278,7 @@ int tmio_mmc_host_runtime_suspend(struct device *dev)
> >> >
> >> >         mmc_retune_timer_stop(host->mmc);
> >> >         mmc_retune_needed(host->mmc);
> >> > +       host->use_saved_taps = true;
> >>
> >> I don't think you should trigger a re-tune here at all. Moreover you
> >> don't need to keep track of whether you have valid tap values by using
> >> the ->use_saved_taps variable, the mmc core deals with this for you.
> >>
> >> Instead, you should restore the tap values by invoking
> >> host->select_tuning() from the ->runtime_resume() callback, although
> >> only when host->mmc->can_retune == 1. We should probably add a helper
> >> function in the mmc core for this check, instead of accessing
> >> "can_retune" directly.
> >
> > Thanks, I was wondering what the best way to handle this is.
> >
> > I tried your suggestion above but it seems that host->mmc->can_retune is
> > zero when ->runtime_resume() is called because of the following call paths:
> >
> > a) _mmc_sd_suspend()
> >      -> mmc_power_off()
> >         -> mmc_set_initial_state()
> >            -> mmc_retune_disable
> >    and
> > b) mmc_sd_runtime_resume()
> >      -> mmc_power_up
> >         -> mmc_set_initial_state()
> >            -> mmc_retune_disable
> >
> >> >         tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL);
> 
> This is exactly what shall happen :-)
> 
> If the mmc core suspends the card, it means it will also re-initialize
> the card when resuming it. In that way the regular tuning sequence
> will take place as a part of re-initialization of the card, so you
> definitely must not restore tap values in these case. That is why you
> should be using the can_retune to know when to restore the values.

Understood. In that case things are working as expected.

> > I plan to repost this patchset without the tap restoration code.
> > This is because I have a number of changes locally, in particular
> > to address other parts of your review, and I would like
> > them to see the light of day.
> >
> > I will mark the tap restoration as a TODO item which we can address
> > before merging the code in question.
> 
> It shouldn't be that hard to implement this, so I rather see that we
> get this right from the beginning.
> 
> As matter of fact it probably requires less code than you had in your
> initial approach, especially since I don't think you need to deal with
> anything tuning related in the ->runtime_suspend() callback, but only
> in ->runtime_resume().

Got it.

Re: [PATCH v4 2/4] mmc: tmio: Add tuning support

[Simon Horman]

On Thu, Sep 01, 2016 at 08:46:44AM +0200, Simon Horman wrote:
> On Wed, Aug 31, 2016 at 09:38:40AM +0200, Ulf Hansson wrote:
> > On 30 August 2016 at 22:51, Simon Horman <horms@verge.net.au> wrote:
> > > On Mon, Aug 29, 2016 at 04:05:55PM +0200, Ulf Hansson wrote:
> > >> On 29 August 2016 at 14:05, Simon Horman <horms@verge.net.au> wrote:
> > >> > On Fri, Aug 26, 2016 at 10:01:35AM +0200, Ulf Hansson wrote:
> > >> >> On 25 August 2016 at 14:04, Simon Horman <horms@verge.net.au> wrote:
> > >> >> > On Tue, Aug 23, 2016 at 05:02:56PM +0200, Ulf Hansson wrote:
> > >> >
> > >> > ...
> > >
> > > ...
> > >
> > >> > @@ -1271,6 +1278,7 @@ int tmio_mmc_host_runtime_suspend(struct device *dev)
> > >> >
> > >> >         mmc_retune_timer_stop(host->mmc);
> > >> >         mmc_retune_needed(host->mmc);
> > >> > +       host->use_saved_taps = true;
> > >>
> > >> I don't think you should trigger a re-tune here at all. Moreover you
> > >> don't need to keep track of whether you have valid tap values by using
> > >> the ->use_saved_taps variable, the mmc core deals with this for you.
> > >>
> > >> Instead, you should restore the tap values by invoking
> > >> host->select_tuning() from the ->runtime_resume() callback, although
> > >> only when host->mmc->can_retune == 1. We should probably add a helper
> > >> function in the mmc core for this check, instead of accessing
> > >> "can_retune" directly.
> > >
> > > Thanks, I was wondering what the best way to handle this is.
> > >
> > > I tried your suggestion above but it seems that host->mmc->can_retune is
> > > zero when ->runtime_resume() is called because of the following call paths:
> > >
> > > a) _mmc_sd_suspend()
> > >      -> mmc_power_off()
> > >         -> mmc_set_initial_state()
> > >            -> mmc_retune_disable
> > >    and
> > > b) mmc_sd_runtime_resume()
> > >      -> mmc_power_up
> > >         -> mmc_set_initial_state()
> > >            -> mmc_retune_disable
> > >
> > >> >         tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL);
> > 
> > This is exactly what shall happen :-)
> > 
> > If the mmc core suspends the card, it means it will also re-initialize
> > the card when resuming it. In that way the regular tuning sequence
> > will take place as a part of re-initialization of the card, so you
> > definitely must not restore tap values in these case. That is why you
> > should be using the can_retune to know when to restore the values.
> 
> Understood. In that case things are working as expected.

I do have one question. It seems to me that  host->mmc->can_retune == 1 on
resume. If so, when would the saved tap values be used?  I feel that I am
missing something here.

> > > I plan to repost this patchset without the tap restoration code.
> > > This is because I have a number of changes locally, in particular
> > > to address other parts of your review, and I would like
> > > them to see the light of day.
> > >
> > > I will mark the tap restoration as a TODO item which we can address
> > > before merging the code in question.
> > 
> > It shouldn't be that hard to implement this, so I rather see that we
> > get this right from the beginning.
> > 
> > As matter of fact it probably requires less code than you had in your
> > initial approach, especially since I don't think you need to deal with
> > anything tuning related in the ->runtime_suspend() callback, but only
> > in ->runtime_resume().
> 
> Got it.

Re: [PATCH v4 2/4] mmc: tmio: Add tuning support

[Ulf Hansson]

On 1 September 2016 at 10:37, Simon Horman <horms@verge.net.au> wrote:
> On Thu, Sep 01, 2016 at 08:46:44AM +0200, Simon Horman wrote:
>> On Wed, Aug 31, 2016 at 09:38:40AM +0200, Ulf Hansson wrote:
>> > On 30 August 2016 at 22:51, Simon Horman <horms@verge.net.au> wrote:
>> > > On Mon, Aug 29, 2016 at 04:05:55PM +0200, Ulf Hansson wrote:
>> > >> On 29 August 2016 at 14:05, Simon Horman <horms@verge.net.au> wrote:
>> > >> > On Fri, Aug 26, 2016 at 10:01:35AM +0200, Ulf Hansson wrote:
>> > >> >> On 25 August 2016 at 14:04, Simon Horman <horms@verge.net.au> wrote:
>> > >> >> > On Tue, Aug 23, 2016 at 05:02:56PM +0200, Ulf Hansson wrote:
>> > >> >
>> > >> > ...
>> > >
>> > > ...
>> > >
>> > >> > @@ -1271,6 +1278,7 @@ int tmio_mmc_host_runtime_suspend(struct device *dev)
>> > >> >
>> > >> >         mmc_retune_timer_stop(host->mmc);
>> > >> >         mmc_retune_needed(host->mmc);
>> > >> > +       host->use_saved_taps = true;
>> > >>
>> > >> I don't think you should trigger a re-tune here at all. Moreover you
>> > >> don't need to keep track of whether you have valid tap values by using
>> > >> the ->use_saved_taps variable, the mmc core deals with this for you.
>> > >>
>> > >> Instead, you should restore the tap values by invoking
>> > >> host->select_tuning() from the ->runtime_resume() callback, although
>> > >> only when host->mmc->can_retune == 1. We should probably add a helper
>> > >> function in the mmc core for this check, instead of accessing
>> > >> "can_retune" directly.
>> > >
>> > > Thanks, I was wondering what the best way to handle this is.
>> > >
>> > > I tried your suggestion above but it seems that host->mmc->can_retune is
>> > > zero when ->runtime_resume() is called because of the following call paths:
>> > >
>> > > a) _mmc_sd_suspend()
>> > >      -> mmc_power_off()
>> > >         -> mmc_set_initial_state()
>> > >            -> mmc_retune_disable
>> > >    and
>> > > b) mmc_sd_runtime_resume()
>> > >      -> mmc_power_up
>> > >         -> mmc_set_initial_state()
>> > >            -> mmc_retune_disable
>> > >
>> > >> >         tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL);
>> >
>> > This is exactly what shall happen :-)
>> >
>> > If the mmc core suspends the card, it means it will also re-initialize
>> > the card when resuming it. In that way the regular tuning sequence
>> > will take place as a part of re-initialization of the card, so you
>> > definitely must not restore tap values in these case. That is why you
>> > should be using the can_retune to know when to restore the values.
>>
>> Understood. In that case things are working as expected.
>
> I do have one question. It seems to me that  host->mmc->can_retune == 1 on
> resume. If so, when would the saved tap values be used?  I feel that I am
> missing something here.

There are different "resumes" here, so this gets a bit tricky to
discuss around. Let me give it another try.

First, let's assume we have a card inserted, initialized and tuned.
This also means we have a cache of tuning (tap) values in the host.

When the *host* runtime PM suspends, which isn't related to when the
card suspends, typically the card remains powered/initialized. Then,
when the host runtime PM resumes the tuning values can be restored in
the host controller, as those should still be working because the card
has remained initialized.

When the card system PM suspends, the card will be put to sleep/power
off (can_retune == 0). Sooner or later the host also becomes runtime
PM suspended in this sequence.

When the card system PM resumes, the card will be woken up from sleep,
power on and the re-initializations starts. Before the
re-initializations starts, the host will be runtime PM resumed, but
since can_retune == 0, no tuning values will be restored (correct).
When the re-initialization sequence completes of the card, can_retune
== 1 as a new tuning sequence has also been completed. This also means
refreshed cached tunings values in the host, which it can use the next
time it will be runtime PM resumed.

Hope this made more sense now.

[...]

Kind regards
Uffe

Re: [PATCH v4 2/4] mmc: tmio: Add tuning support

[Simon Horman]

On Thu, Sep 01, 2016 at 11:55:27AM +0200, Ulf Hansson wrote:
> On 1 September 2016 at 10:37, Simon Horman <horms@verge.net.au> wrote:
> > On Thu, Sep 01, 2016 at 08:46:44AM +0200, Simon Horman wrote:
> >> On Wed, Aug 31, 2016 at 09:38:40AM +0200, Ulf Hansson wrote:
> >> > On 30 August 2016 at 22:51, Simon Horman <horms@verge.net.au> wrote:
> >> > > On Mon, Aug 29, 2016 at 04:05:55PM +0200, Ulf Hansson wrote:
> >> > >> On 29 August 2016 at 14:05, Simon Horman <horms@verge.net.au> wrote:
> >> > >> > On Fri, Aug 26, 2016 at 10:01:35AM +0200, Ulf Hansson wrote:
> >> > >> >> On 25 August 2016 at 14:04, Simon Horman <horms@verge.net.au> wrote:
> >> > >> >> > On Tue, Aug 23, 2016 at 05:02:56PM +0200, Ulf Hansson wrote:
> >> > >> >
> >> > >> > ...
> >> > >
> >> > > ...
> >> > >
> >> > >> > @@ -1271,6 +1278,7 @@ int tmio_mmc_host_runtime_suspend(struct device *dev)
> >> > >> >
> >> > >> >         mmc_retune_timer_stop(host->mmc);
> >> > >> >         mmc_retune_needed(host->mmc);
> >> > >> > +       host->use_saved_taps = true;
> >> > >>
> >> > >> I don't think you should trigger a re-tune here at all. Moreover you
> >> > >> don't need to keep track of whether you have valid tap values by using
> >> > >> the ->use_saved_taps variable, the mmc core deals with this for you.
> >> > >>
> >> > >> Instead, you should restore the tap values by invoking
> >> > >> host->select_tuning() from the ->runtime_resume() callback, although
> >> > >> only when host->mmc->can_retune == 1. We should probably add a helper
> >> > >> function in the mmc core for this check, instead of accessing
> >> > >> "can_retune" directly.
> >> > >
> >> > > Thanks, I was wondering what the best way to handle this is.
> >> > >
> >> > > I tried your suggestion above but it seems that host->mmc->can_retune is
> >> > > zero when ->runtime_resume() is called because of the following call paths:
> >> > >
> >> > > a) _mmc_sd_suspend()
> >> > >      -> mmc_power_off()
> >> > >         -> mmc_set_initial_state()
> >> > >            -> mmc_retune_disable
> >> > >    and
> >> > > b) mmc_sd_runtime_resume()
> >> > >      -> mmc_power_up
> >> > >         -> mmc_set_initial_state()
> >> > >            -> mmc_retune_disable
> >> > >
> >> > >> >         tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL);
> >> >
> >> > This is exactly what shall happen :-)
> >> >
> >> > If the mmc core suspends the card, it means it will also re-initialize
> >> > the card when resuming it. In that way the regular tuning sequence
> >> > will take place as a part of re-initialization of the card, so you
> >> > definitely must not restore tap values in these case. That is why you
> >> > should be using the can_retune to know when to restore the values.
> >>
> >> Understood. In that case things are working as expected.
> >
> > I do have one question. It seems to me that  host->mmc->can_retune == 1 on
> > resume. If so, when would the saved tap values be used?  I feel that I am
> > missing something here.
> 
> There are different "resumes" here, so this gets a bit tricky to
> discuss around. Let me give it another try.
> 
> First, let's assume we have a card inserted, initialized and tuned.
> This also means we have a cache of tuning (tap) values in the host.
> 
> When the *host* runtime PM suspends, which isn't related to when the
> card suspends, typically the card remains powered/initialized. Then,
> when the host runtime PM resumes the tuning values can be restored in
> the host controller, as those should still be working because the card
> has remained initialized.
> 
> When the card system PM suspends, the card will be put to sleep/power
> off (can_retune == 0). Sooner or later the host also becomes runtime
> PM suspended in this sequence.
> 
> When the card system PM resumes, the card will be woken up from sleep,
> power on and the re-initializations starts. Before the
> re-initializations starts, the host will be runtime PM resumed, but
> since can_retune == 0, no tuning values will be restored (correct).
> When the re-initialization sequence completes of the card, can_retune
> == 1 as a new tuning sequence has also been completed. This also means
> refreshed cached tunings values in the host, which it can use the next
> time it will be runtime PM resumed.
> 
> Hope this made more sense now.

Thanks for your detailed explanation. I understand now.