[PATCH/RFC 0/3] UHS-I SDR-104 support for sh_mobile_sdhi

[PATCH/RFC 0/3] 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 a merge of:
* The next branch of the mmc tree
* The renesas-next-20160427v2-v4.6-rc1 tag of the renesas tree
* The sh-pfc-for-v4.7-tag1 tag of the renesas-drivers tree

The first two 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 branch
of the renesas tree.

A description of testing performed follows:

* Environment

  SoC/Board: r8a7790/Lager
  SanDisk Card: SanDisk Ultra 64Gb microSDXC UHS-1
                Identifier on Packaging: SDSQUNC-064G-GN6MA
  Samsung Card: Samsung 32Gb microSDHC Card Class 10 UHS-1 U3
                ModelCode: MB-MG32EA/FFP

* Wit these patches (topic/sdr104 branch)

  # mount -t debugfs none /sys/kernel/debug
  # cat /sys/kernel/debug/mmc1/ios

  clock:          195000000 Hz
  vdd:            21 (3.3 ~ 3.4 V)
  bus mode:       2 (push-pull)
  chip select:    0 (don't care)
  power mode:     2 (on)
  bus width:      2 (4 bits)
  timing spec:    6 (sd uhs SDR104)
  signal voltage: 1 (1.80 V)
  driver type:    0 (driver type B)

  SanDisk Card:
  # dd bs=1M count=512 iflag=direct if=/dev/mmcblk1 of=/dev/null
  536870912 bytes (537 MB) copied, 13.6713 s, 39.3 MB/s

  Samsung Card:
  # dd bs=1M count=512 iflag=direct if=/dev/mmcblk1 of=/dev/null
  536870912 bytes (537 MB) copied, 16.4952 s, 32.5 MB/s


* Without these patches (base of topic/sdr104 branch)

  # mount -t debugfs none /sys/kernel/debug
  # cat /sys/kernel/debug/mmc1/ios

  clock:          100000000 Hz
  vdd:            21 (3.3 ~ 3.4 V)
  bus mode:       2 (push-pull)
  chip select:    0 (don't care)
  power mode:     2 (on)
  bus width:      2 (4 bits)
  timing spec:    5 (sd uhs SDR50)
  signal voltage: 1 (1.80 V)
  driver type:    0 (driver type B)

  SanDisk Card:
  # dd bs=1M count=512 iflag=direct if=/dev/mmcblk1 of=/dev/null
  536870912 bytes (537 MB) copied, 17.7253 s, 30.3 MB/s

  Samsung Card:
  # dd bs=1M count=512 iflag=direct if=/dev/mmcblk1 of=/dev/null
  536870912 bytes (537 MB) copied, 20.7866 s, 25.8 MB/s



Ai Kyuse (2):
  mmc: tmio: Add tuning support
  mmc: sh_mobile_sdhi: Add tuning support

Simon Horman (1):
  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         |  10 ++
 drivers/mmc/host/tmio_mmc_pio.c     | 249 ++++++++++++++++++++++++++++++++--
 include/linux/mfd/tmio.h            |   3 +
 5 files changed, 518 insertions(+), 9 deletions(-)

-- 
2.1.4

[PATCH/RFC 1/3] 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>
---
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]
---
 drivers/mmc/host/tmio_mmc.h     |  10 ++
 drivers/mmc/host/tmio_mmc_pio.c | 249 ++++++++++++++++++++++++++++++++++++++--
 include/linux/mfd/tmio.h        |   3 +
 3 files changed, 254 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 1aac2ad8edf2..cacc64c87fa0 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -19,6 +19,7 @@
 #define TMIO_MMC_H
 
 #include <linux/dmaengine.h>
+#include <linux/completion.h>
 #include <linux/highmem.h>
 #include <linux/mutex.h>
 #include <linux/pagemap.h>
@@ -150,6 +151,9 @@ struct tmio_mmc_host {
 	struct mutex		ios_lock;	/* protect set_ios() context */
 	bool			native_hotplug;
 	bool			sdio_irq_enabled;
+	u32			scc_tappos;
+	bool			done_tuning;
+	struct completion	completion;
 
 	int (*write16_hook)(struct tmio_mmc_host *host, int addr);
 	int (*clk_enable)(struct tmio_mmc_host *host);
@@ -160,6 +164,12 @@ struct tmio_mmc_host {
 			      unsigned int direction, int blk_size);
 	int (*start_signal_voltage_switch)(struct mmc_host *mmc,
 					   struct mmc_ios *ios);
+	bool (*inquiry_tuning)(struct tmio_mmc_host *host);
+	unsigned int (*init_tuning)(struct tmio_mmc_host *host);
+	int (*prepare_tuning)(struct tmio_mmc_host *host, unsigned long tap);
+	int (*select_tuning)(struct tmio_mmc_host *host, unsigned long *tap);
+	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 f44e2ab7aea2..4e6d80ad7bac 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>
@@ -277,6 +278,8 @@ static void tmio_mmc_finish_request(struct tmio_mmc_host *host)
 {
 	struct mmc_request *mrq;
 	unsigned long flags;
+	bool result;
+	struct mmc_command *cmd = host->cmd;
 
 	spin_lock_irqsave(&host->lock, flags);
 
@@ -290,7 +293,9 @@ static void tmio_mmc_finish_request(struct tmio_mmc_host *host)
 	host->data = NULL;
 	host->force_pio = false;
 
-	cancel_delayed_work(&host->delayed_reset_work);
+	if (!(host->inquiry_tuning && host->inquiry_tuning(host) &&
+	      !host->done_tuning) || cmd != mrq->sbc)
+		cancel_delayed_work(&host->delayed_reset_work);
 
 	host->mrq = NULL;
 	spin_unlock_irqrestore(&host->lock, flags);
@@ -298,6 +303,29 @@ 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->inquiry_tuning && host->inquiry_tuning(host) &&
+	     !host->done_tuning) {
+		/* call retuning() to clear SCC error bit */
+		if (host->retuning)
+			host->retuning(host);
+		/* finish processing tuning request */
+		complete(&host->completion);
+		return;
+	}
+
+	/* Check retuning */
+	if (host->retuning && host->done_tuning) {
+		result = host->retuning(host);
+		if (result || (mrq->cmd->error == -EILSEQ))
+			host->done_tuning = false;
+	}
+
+	if (cmd == mrq->sbc) {
+		/* finish SET_BLOCK_COUNT request */
+		complete(&host->completion);
+		return;
+	}
+
 	mmc_request_done(host->mmc, mrq);
 }
 
@@ -363,7 +391,8 @@ static int tmio_mmc_start_command(struct tmio_mmc_host *host, struct mmc_command
 			 * multiple block transfer
 			 */
 			if ((host->pdata->flags & TMIO_MMC_HAVE_CMD12_CTRL) &&
-			    (cmd->opcode == SD_IO_RW_EXTENDED))
+			    ((cmd->opcode == SD_IO_RW_EXTENDED) ||
+			     host->mrq->sbc))
 				c |= NO_CMD12_ISSUE;
 		}
 		if (data->flags & MMC_DATA_READ)
@@ -520,7 +549,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 +558,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 +609,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 +628,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 +698,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;
 	}
 
@@ -753,6 +785,157 @@ static int tmio_mmc_start_data(struct tmio_mmc_host *host,
 	return 0;
 }
 
+#define TMIO_MMC_MAX_TUNING_LOOP 40
+
+static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
+{
+	struct tmio_mmc_host *host = mmc_priv(mmc);
+	struct mmc_ios *ios = &mmc->ios;
+
+	unsigned long timeout, val;
+	unsigned long *tap;
+	int tuning_loop_counter = TMIO_MMC_MAX_TUNING_LOOP;
+	int ret, timeleft;
+
+	struct mmc_request mrq = {NULL};
+	struct mmc_command cmd = {0};
+	struct mmc_data data = {0};
+	struct scatterlist sg;
+	u8 *data_buf;
+	unsigned int num, tm = CMDREQ_TIMEOUT;
+	unsigned long flags;
+
+	if ((ios->timing != MMC_TIMING_UHS_SDR50 &&
+	     ios->timing != MMC_TIMING_UHS_SDR104) ||
+	    (host->inquiry_tuning && !host->inquiry_tuning(host)) ||
+	    host->done_tuning || !host->init_tuning)
+		return 0;
+
+	num = host->init_tuning(host);
+
+	tap = kmalloc(num * 2, GFP_KERNEL);
+	if (tap == NULL) {
+		ret = -ENOMEM;
+		goto err_tap;
+	}
+
+	data_buf = kmalloc(64, GFP_KERNEL);
+	if (data_buf == NULL) {
+		ret = -ENOMEM;
+		goto err_data;
+	}
+
+	val = 0;
+
+	/*
+	 * Issue CMD19 repeatedly till Execute Tuning is set to 0 or the number
+	 * of loops reaches 40 times or a timeout of 150ms occurs.
+	 */
+	timeout = 150;
+	do {
+		if (host->prepare_tuning)
+			host->prepare_tuning(host, val % num);
+
+		if (!tuning_loop_counter && !timeout)
+			break;
+
+		/*
+		 * In response to CMD19, the card sends 64 bytes of tuning
+		 * block to the Host Controller. So we set the block size
+		 * to 64 here.
+		 */
+
+		spin_lock_irqsave(&host->lock, flags);
+		init_completion(&host->completion);
+		mrq.cmd = &cmd;
+		mrq.data = &data;
+
+		cmd.opcode = opcode;
+		cmd.arg = 0;
+		cmd.flags = MMC_RSP_R1 | MMC_CMD_ADTC;
+		cmd.retries = 0;
+		cmd.error = 0;
+
+		data.blksz = 64;
+		data.blocks = 1;
+		data.flags = MMC_DATA_READ;
+		data.sg = &sg;
+		data.sg_len = 1;
+		data.error = 0;
+
+		sg_init_one(&sg, data_buf, 64);
+
+		host->mrq = &mrq;
+
+		spin_unlock_irqrestore(&host->lock, flags);
+
+		ret = tmio_mmc_start_data(host, mrq.data);
+		if (ret)
+			goto out;
+
+		ret = tmio_mmc_start_command(host, mrq.cmd);
+		if (ret)
+			goto out;
+
+		timeleft = wait_for_completion_timeout(&host->completion,
+						       msecs_to_jiffies(tm));
+		if (timeleft < 0) {
+			ret = timeleft;
+			goto out;
+		}
+
+		if (!timeleft) {
+			ret = -ETIMEDOUT;
+			goto out;
+		}
+
+		/* Check CRC error */
+		if (cmd.error && cmd.error != -EILSEQ) {
+			ret = cmd.error;
+			goto out;
+		}
+		if (data.error && data.error != -EILSEQ) {
+			ret = data.error;
+			goto out;
+		}
+
+		tap[val] = (cmd.error | data.error);
+
+		val++;
+		tuning_loop_counter--;
+		timeout--;
+		mdelay(1);
+	} while ((val < (num * 2)) && (tuning_loop_counter || timeout));
+
+	/*
+	 * The Host Driver has exhausted the maximum number of loops allowed,
+	 * so use fixed sampling frequency.
+	 */
+	if (tuning_loop_counter || timeout) {
+		if (host->select_tuning) {
+			ret = host->select_tuning(host, tap);
+			if (ret < 0)
+				goto out;
+		}
+		host->done_tuning = true;
+	} else {
+		dev_warn(&host->pdev->dev, ": Tuning procedure failed\n");
+		ret = -EIO;
+		goto out;
+	}
+
+out:
+	kfree(data_buf);
+err_data:
+	kfree(tap);
+err_tap:
+	if (ret < 0 && host->hw_reset)
+		host->hw_reset(host);
+
+	return ret;
+
+}
+
 /* Process requests from the MMC layer */
 static void tmio_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
 {
@@ -778,6 +961,43 @@ static void tmio_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
 
 	spin_unlock_irqrestore(&host->lock, flags);
 
+	if (host->inquiry_tuning && host->inquiry_tuning(host) &&
+	    !host->done_tuning) {
+		/* Start retuning */
+		ret = tmio_mmc_execute_tuning(mmc, MMC_SEND_TUNING_BLOCK);
+		if (ret)
+			goto fail;
+		/* Restore request */
+		host->mrq = mrq;
+	}
+
+	if (mrq->sbc) {
+		init_completion(&host->completion);
+		ret = tmio_mmc_start_command(host, mrq->sbc);
+		if (ret)
+			goto fail;
+		ret = wait_for_completion_timeout(&host->completion,
+					msecs_to_jiffies(CMDREQ_TIMEOUT));
+		if (ret < 0)
+			goto fail;
+		if (!ret) {
+			ret = -ETIMEDOUT;
+			goto fail;
+		}
+		host->last_req_ts = jiffies;
+		host->mrq = mrq;
+		if (host->inquiry_tuning && host->inquiry_tuning(host) &&
+		    !host->done_tuning) {
+			/* Start retuning */
+			ret = tmio_mmc_execute_tuning(mmc,
+						      MMC_SEND_TUNING_BLOCK);
+			if (ret)
+				goto fail;
+			/* Restore request */
+			host->mrq = mrq;
+		}
+	}
+
 	if (mrq->data) {
 		ret = tmio_mmc_start_data(host, mrq->data);
 		if (ret)
@@ -967,6 +1187,16 @@ static int tmio_mmc_card_busy(struct mmc_host *mmc)
 	return !(sd_ctrl_read16_and_16_as_32(host, CTL_STATUS) & TMIO_STAT_DAT0);
 }
 
+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);
+
+	host->done_tuning = false;
+}
+
 static struct mmc_host_ops tmio_mmc_ops = {
 	.request	= tmio_mmc_request,
 	.set_ios	= tmio_mmc_set_ios,
@@ -975,6 +1205,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)
@@ -1084,6 +1316,7 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host,
 	mmc->max_req_size = mmc->max_blk_size * mmc->max_blk_count;
 	mmc->max_seg_size = mmc->max_req_size;
 
+	_host->done_tuning = false;
 	_host->native_hotplug = !(pdata->flags & TMIO_MMC_USE_GPIO_CD ||
 				  mmc->caps & MMC_CAP_NEEDS_POLL ||
 				  mmc->caps & MMC_CAP_NONREMOVABLE ||
diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
index 7a26286db895..6c22b21f2d73 100644
--- a/include/linux/mfd/tmio.h
+++ b/include/linux/mfd/tmio.h
@@ -104,6 +104,9 @@
  */
 #define TMIO_MMC_CLK_ACTUAL		(1 << 10)
 
+/* Some controllers have UHS-I sampling clock controller */
+#define TMIO_MMC_HAS_UHS_SCC		(1 << 11)
+
 int tmio_core_mmc_enable(void __iomem *cnf, int shift, unsigned long base);
 int tmio_core_mmc_resume(void __iomem *cnf, int shift, unsigned long base);
 void tmio_core_mmc_pwr(void __iomem *cnf, int shift, int state);
-- 
2.1.4

RE: [PATCH/RFC 1/3] mmc: tmio: Add tuning support

[Yoshihiro Shimoda]

Hi Simon-san,

> From: Behalf Of Simon Horman
> Sent: Tuesday, May 10, 2016 2:52 PM
> 
> 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>

I have some minor comments :)

< snip >
> @@ -753,6 +785,157 @@ static int tmio_mmc_start_data(struct tmio_mmc_host *host,
>  	return 0;
>  }
> 
> +#define TMIO_MMC_MAX_TUNING_LOOP 40
> +
> +static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
> +{
> +	struct tmio_mmc_host *host = mmc_priv(mmc);
> +	struct mmc_ios *ios = &mmc->ios;
> +
> +	unsigned long timeout, val;
> +	unsigned long *tap;
> +	int tuning_loop_counter = TMIO_MMC_MAX_TUNING_LOOP;
< snip >
> +	/*
> +	 * Issue CMD19 repeatedly till Execute Tuning is set to 0 or the number
> +	 * of loops reaches 40 times or a timeout of 150ms occurs.
> +	 */
> +	timeout = 150;

The tuning_loop_counter is 40 and timeout is 150.
However,

> +	do {
> +		if (host->prepare_tuning)
> +			host->prepare_tuning(host, val % num);
> +
> +		if (!tuning_loop_counter && !timeout)
> +			break;

< snip >

> +		tuning_loop_counter--;
> +		timeout--;
> +		mdelay(1);
> +	} while ((val < (num * 2)) && (tuning_loop_counter || timeout));

They will be decreased by 1. So, I think we don't need either variable.

> +	/*
> +	 * The Host Driver has exhausted the maximum number of loops allowed,
> +	 * so use fixed sampling frequency.
> +	 */
> +	if (tuning_loop_counter || timeout) {
> +		if (host->select_tuning) {
> +			ret = host->select_tuning(host, tap);
> +			if (ret < 0)
> +				goto out;
> +		}
> +		host->done_tuning = true;
> +	} else {
> +		dev_warn(&host->pdev->dev, ": Tuning procedure failed\n");

The first 2 charactors ": " is strange to me.

> +		ret = -EIO;
> +		goto out;
> +	}
> +
> +out:
> +	kfree(data_buf);
> +err_data:
> +	kfree(tap);
> +err_tap:
> +	if (ret < 0 && host->hw_reset)
> +		host->hw_reset(host);

We can use tmio_mmc_hw_reset() of this patch.
Then, we can remove the condition of "host->hw_reset".

Best regards,
Yoshihiro Shimoda

> +	return ret;
> +
> +}
> +
>  /* Process requests from the MMC layer */
>  static void tmio_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  {
> @@ -778,6 +961,43 @@ static void tmio_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
> 
>  	spin_unlock_irqrestore(&host->lock, flags);
> 
> +	if (host->inquiry_tuning && host->inquiry_tuning(host) &&
> +	    !host->done_tuning) {
> +		/* Start retuning */
> +		ret = tmio_mmc_execute_tuning(mmc, MMC_SEND_TUNING_BLOCK);
> +		if (ret)
> +			goto fail;
> +		/* Restore request */
> +		host->mrq = mrq;
> +	}
> +
> +	if (mrq->sbc) {
> +		init_completion(&host->completion);
> +		ret = tmio_mmc_start_command(host, mrq->sbc);
> +		if (ret)
> +			goto fail;
> +		ret = wait_for_completion_timeout(&host->completion,
> +					msecs_to_jiffies(CMDREQ_TIMEOUT));
> +		if (ret < 0)
> +			goto fail;
> +		if (!ret) {
> +			ret = -ETIMEDOUT;
> +			goto fail;
> +		}
> +		host->last_req_ts = jiffies;
> +		host->mrq = mrq;
> +		if (host->inquiry_tuning && host->inquiry_tuning(host) &&
> +		    !host->done_tuning) {
> +			/* Start retuning */
> +			ret = tmio_mmc_execute_tuning(mmc,
> +						      MMC_SEND_TUNING_BLOCK);
> +			if (ret)
> +				goto fail;
> +			/* Restore request */
> +			host->mrq = mrq;
> +		}
> +	}
> +
>  	if (mrq->data) {
>  		ret = tmio_mmc_start_data(host, mrq->data);
>  		if (ret)
> @@ -967,6 +1187,16 @@ static int tmio_mmc_card_busy(struct mmc_host *mmc)
>  	return !(sd_ctrl_read16_and_16_as_32(host, CTL_STATUS) & TMIO_STAT_DAT0);
>  }
> 
> +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);
> +
> +	host->done_tuning = false;
> +}
> +
>  static struct mmc_host_ops tmio_mmc_ops = {
>  	.request	= tmio_mmc_request,
>  	.set_ios	= tmio_mmc_set_ios,
> @@ -975,6 +1205,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)
> @@ -1084,6 +1316,7 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host,
>  	mmc->max_req_size = mmc->max_blk_size * mmc->max_blk_count;
>  	mmc->max_seg_size = mmc->max_req_size;
> 
> +	_host->done_tuning = false;
>  	_host->native_hotplug = !(pdata->flags & TMIO_MMC_USE_GPIO_CD ||
>  				  mmc->caps & MMC_CAP_NEEDS_POLL ||
>  				  mmc->caps & MMC_CAP_NONREMOVABLE ||
> diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
> index 7a26286db895..6c22b21f2d73 100644
> --- a/include/linux/mfd/tmio.h
> +++ b/include/linux/mfd/tmio.h
> @@ -104,6 +104,9 @@
>   */
>  #define TMIO_MMC_CLK_ACTUAL		(1 << 10)
> 
> +/* Some controllers have UHS-I sampling clock controller */
> +#define TMIO_MMC_HAS_UHS_SCC		(1 << 11)
> +
>  int tmio_core_mmc_enable(void __iomem *cnf, int shift, unsigned long base);
>  int tmio_core_mmc_resume(void __iomem *cnf, int shift, unsigned long base);
>  void tmio_core_mmc_pwr(void __iomem *cnf, int shift, int state);
> --
> 2.1.4

Re: [PATCH/RFC 1/3] mmc: tmio: Add tuning support

[Simon Horman]

On Thu, May 12, 2016 at 06:12:44AM +0000, Yoshihiro Shimoda wrote:
> Hi Simon-san,
> 
> > From: Behalf Of Simon Horman
> > Sent: Tuesday, May 10, 2016 2:52 PM
> > 
> > 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>
> 
> I have some minor comments :)
> 
> < snip >
> > @@ -753,6 +785,157 @@ static int tmio_mmc_start_data(struct tmio_mmc_host *host,
> >  	return 0;
> >  }
> > 
> > +#define TMIO_MMC_MAX_TUNING_LOOP 40
> > +
> > +static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
> > +{
> > +	struct tmio_mmc_host *host = mmc_priv(mmc);
> > +	struct mmc_ios *ios = &mmc->ios;
> > +
> > +	unsigned long timeout, val;
> > +	unsigned long *tap;
> > +	int tuning_loop_counter = TMIO_MMC_MAX_TUNING_LOOP;
> < snip >
> > +	/*
> > +	 * Issue CMD19 repeatedly till Execute Tuning is set to 0 or the number
> > +	 * of loops reaches 40 times or a timeout of 150ms occurs.
> > +	 */
> > +	timeout = 150;
> 
> The tuning_loop_counter is 40 and timeout is 150.
> However,
> 
> > +	do {
> > +		if (host->prepare_tuning)
> > +			host->prepare_tuning(host, val % num);
> > +
> > +		if (!tuning_loop_counter && !timeout)
> > +			break;
> 
> < snip >
> 
> > +		tuning_loop_counter--;
> > +		timeout--;
> > +		mdelay(1);
> > +	} while ((val < (num * 2)) && (tuning_loop_counter || timeout));
> 
> They will be decreased by 1. So, I think we don't need either variable.

Thanks for bringing this to my attention.

As Wolfram pointed out in another sub-thread it looks like this code
is based on sdhci.c. And I believe that the version in that file has
the issue you raise addressed by:

7ce45e950624 ("mmc: sdhci: SD tuning is broken for some controllers").

I'll update this patch accordingly.

> 
> > +	 * The Host Driver has exhausted the maximum number of loops allowed,
> > +	 * so use fixed sampling frequency.
> > +	 */
> > +	if (tuning_loop_counter || timeout) {
> > +		if (host->select_tuning) {
> > +			ret = host->select_tuning(host, tap);
> > +			if (ret < 0)
> > +				goto out;
> > +		}
> > +		host->done_tuning = true;
> > +	} else {
> > +		dev_warn(&host->pdev->dev, ": Tuning procedure failed\n");
> 
> The first 2 charactors ": " is strange to me.

Yes, thanks for noticing that. I plan to drop ": ".

> > +		ret = -EIO;
> > +		goto out;
> > +	}
> > +
> > +out:
> > +	kfree(data_buf);
> > +err_data:
> > +	kfree(tap);
> > +err_tap:
> > +	if (ret < 0 && host->hw_reset)
> > +		host->hw_reset(host);
> 
> We can use tmio_mmc_hw_reset() of this patch.
> Then, we can remove the condition of "host->hw_reset".

Thanks, will do.

[...]

Re: [PATCH/RFC 1/3] mmc: tmio: Add tuning support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 4262 bytes --]

Hi Simon,

nice you got it working with upstream! Thanks. It still looks a bit too
much like BSP code, though, so we need to clean it up. I found 'git log
--grep=tuning drivers/mmc/host' to be useful to get an idea of current
best practices.

> +	bool (*inquiry_tuning)(struct tmio_mmc_host *host);

Do we really need this? I'd think the core will check that tuning only
gets called when needed.

> -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 +558,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 +609,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 +628,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 +698,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;
>  	}

I wonder if the changes to this tmio_mmc_*_irq are a seperate patch?
I'd think they need a seperate description.

>  
> @@ -753,6 +785,157 @@ static int tmio_mmc_start_data(struct tmio_mmc_host *host,
>  	return 0;
>  }
>  
> +#define TMIO_MMC_MAX_TUNING_LOOP 40
> +
> +static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
> +{

I'd think we can use mmc_send_tuning() from the mmc core here in here.

> +	struct tmio_mmc_host *host = mmc_priv(mmc);
> +	struct mmc_ios *ios = &mmc->ios;
> +
> +	unsigned long timeout, val;
> +	unsigned long *tap;
> +	int tuning_loop_counter = TMIO_MMC_MAX_TUNING_LOOP;
> +	int ret, timeleft;
> +
> +	struct mmc_request mrq = {NULL};
> +	struct mmc_command cmd = {0};
> +	struct mmc_data data = {0};
> +	struct scatterlist sg;
> +	u8 *data_buf;
> +	unsigned int num, tm = CMDREQ_TIMEOUT;
> +	unsigned long flags;
> +
> +	if ((ios->timing != MMC_TIMING_UHS_SDR50 &&
> +	     ios->timing != MMC_TIMING_UHS_SDR104) ||
> +	    (host->inquiry_tuning && !host->inquiry_tuning(host)) ||
> +	    host->done_tuning || !host->init_tuning)
> +		return 0;
> +
> +	num = host->init_tuning(host);
> +
> +	tap = kmalloc(num * 2, GFP_KERNEL);
> +	if (tap == NULL) {
> +		ret = -ENOMEM;
> +		goto err_tap;
> +	}
> +
> +	data_buf = kmalloc(64, GFP_KERNEL);
> +	if (data_buf == NULL) {
> +		ret = -ENOMEM;
> +		goto err_data;
> +	}
> +
> +	val = 0;
> +
> +	/*
> +	 * Issue CMD19 repeatedly till Execute Tuning is set to 0 or the number
> +	 * of loops reaches 40 times or a timeout of 150ms occurs.
> +	 */

Note to self: this is copied from sdhci.c

So far for now!

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH/RFC 1/3] mmc: tmio: Add tuning support

[Simon Horman]

On Thu, May 12, 2016 at 06:50:05PM +0200, Wolfram Sang wrote:
> Hi Simon,
> 
> nice you got it working with upstream! Thanks. It still looks a bit too
> much like BSP code, though, so we need to clean it up. I found 'git log
> --grep=tuning drivers/mmc/host' to be useful to get an idea of current
> best practices.
> 
> > +	bool (*inquiry_tuning)(struct tmio_mmc_host *host);
> 
> Do we really need this? I'd think the core will check that tuning only
> gets called when needed.

Thanks, I'll look into that and in general updating the approach taken
to tuning to reflect that currently taken in mainline.

> > -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 +558,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 +609,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 +628,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 +698,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;
> >  	}
> 
> I wonder if the changes to this tmio_mmc_*_irq are a seperate patch?
> I'd think they need a seperate description.

Yes, I think so.

Looking over this its not entirely clear that they are limited in
usefulness to tuning.

> > @@ -753,6 +785,157 @@ static int tmio_mmc_start_data(struct tmio_mmc_host *host,
> >  	return 0;
> >  }
> >  
> > +#define TMIO_MMC_MAX_TUNING_LOOP 40
> > +
> > +static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
> > +{
> 
> I'd think we can use mmc_send_tuning() from the mmc core here in here.

Yes, having looked over mainline I think that seems likely.

> > +	struct tmio_mmc_host *host = mmc_priv(mmc);
> > +	struct mmc_ios *ios = &mmc->ios;
> > +
> > +	unsigned long timeout, val;
> > +	unsigned long *tap;
> > +	int tuning_loop_counter = TMIO_MMC_MAX_TUNING_LOOP;
> > +	int ret, timeleft;
> > +
> > +	struct mmc_request mrq = {NULL};
> > +	struct mmc_command cmd = {0};
> > +	struct mmc_data data = {0};
> > +	struct scatterlist sg;
> > +	u8 *data_buf;
> > +	unsigned int num, tm = CMDREQ_TIMEOUT;
> > +	unsigned long flags;
> > +
> > +	if ((ios->timing != MMC_TIMING_UHS_SDR50 &&
> > +	     ios->timing != MMC_TIMING_UHS_SDR104) ||
> > +	    (host->inquiry_tuning && !host->inquiry_tuning(host)) ||
> > +	    host->done_tuning || !host->init_tuning)
> > +		return 0;
> > +
> > +	num = host->init_tuning(host);
> > +
> > +	tap = kmalloc(num * 2, GFP_KERNEL);
> > +	if (tap == NULL) {
> > +		ret = -ENOMEM;
> > +		goto err_tap;
> > +	}
> > +
> > +	data_buf = kmalloc(64, GFP_KERNEL);
> > +	if (data_buf == NULL) {
> > +		ret = -ENOMEM;
> > +		goto err_data;
> > +	}
> > +
> > +	val = 0;
> > +
> > +	/*
> > +	 * Issue CMD19 repeatedly till Execute Tuning is set to 0 or the number
> > +	 * of loops reaches 40 times or a timeout of 150ms occurs.
> > +	 */
> 
> Note to self: this is copied from sdhci.c

It also seems to be copied from an old version of sdhci.c.
So at the very least there seem some updates to be incorporated. 

Re: [PATCH/RFC 1/3] mmc: tmio: Add tuning support

[Simon Horman]

On Fri, May 13, 2016 at 11:28:11AM +0900, Simon Horman wrote:
> On Thu, May 12, 2016 at 06:50:05PM +0200, Wolfram Sang wrote:
> > Hi Simon,
> > 
> > nice you got it working with upstream! Thanks. It still looks a bit too
> > much like BSP code, though, so we need to clean it up. I found 'git log
> > --grep=tuning drivers/mmc/host' to be useful to get an idea of current
> > best practices.
> > 
> > > +	bool (*inquiry_tuning)(struct tmio_mmc_host *host);
> > 
> > Do we really need this? I'd think the core will check that tuning only
> > gets called when needed.
> 
> Thanks, I'll look into that and in general updating the approach taken
> to tuning to reflect that currently taken in mainline.

[...]

> > Note to self: this is copied from sdhci.c
> 
> It also seems to be copied from an old version of sdhci.c.
> So at the very least there seem some updates to be incorporated. 

On closer inspection I don't think that the loop in sdhci_execute_tuning()
implements the correct logic for the R-Car SCC. I'll look into reworking
it accordingly.

[PATCH/RFC 2/3] mmc: sh_mobile_sdhi: Add tuning support

[Simon Horman]

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

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

Signed-off-by: Ai Kyuse <ai.kyuse.uw@renesas.com>
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
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]
---
 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 5309c73be1f0..e74bbdad05f4 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -41,6 +41,11 @@
 
 #define host_to_priv(host) container_of((host)->pdata, struct sh_mobile_sdhi, mmc_data)
 
+struct sh_mobile_sdhi_scc {
+	unsigned long clk;	/* clock for SDR104 */
+	u32 tap;		/* sampling clock position for SDR104 */
+};
+
 struct sh_mobile_sdhi_of_data {
 	unsigned long tmio_flags;
 	unsigned long capabilities;
@@ -48,6 +53,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 = {
@@ -60,12 +68,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 = 156000000,
+		.tap = 0x00000703,
+	},
+	{
+		.clk = 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 = {
@@ -207,6 +230,18 @@ static void sh_mobile_sdhi_clk_disable(struct tmio_mmc_host *host)
 	clk_disable_unprepare(priv->clk);
 }
 
+static void sh_mobile_sdhi_set_clk_div(struct platform_device *pdev, int state)
+{
+	struct mmc_host *mmc = platform_get_drvdata(pdev);
+	struct tmio_mmc_host *host = mmc_priv(mmc);
+
+	if (state) {
+		sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, ~0x0100 &
+				sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
+		sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, 0x00ff);
+	}
+}
+
 static int sh_mobile_sdhi_start_signal_voltage_switch(struct mmc_host *mmc,
 						      struct mmc_ios *ios)
 {
@@ -241,6 +276,202 @@ 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, int addr)
+{
+	struct platform_device *pdev = host->pdev;
+	const struct of_device_id *of_id =
+		of_match_device(sh_mobile_sdhi_of_match, &pdev->dev);
+	const struct sh_mobile_sdhi_of_data *of_data = of_id->data;
+
+	return readl(host->ctl + of_data->scc_offset +
+		     (addr << host->bus_shift));
+}
+
+static inline void sd_scc_write32(struct tmio_mmc_host *host, int addr,
+				  u32 val)
+{
+	struct platform_device *pdev = host->pdev;
+	const struct of_device_id *of_id =
+		of_match_device(sh_mobile_sdhi_of_match, &pdev->dev);
+	const struct sh_mobile_sdhi_of_data *of_data = of_id->data;
+
+	writel(val, host->ctl + of_data->scc_offset +
+	       (addr << host->bus_shift));
+}
+
+static bool sh_mobile_sdhi_inquiry_tuning(struct tmio_mmc_host *host)
+{
+	/* SDHI should be tuning only SDR104 */
+	if (host->mmc->ios.timing == MMC_TIMING_UHS_SDR104)
+		return true;
+	else
+		return false;
+}
+
+static unsigned int sh_mobile_sdhi_init_tuning(struct tmio_mmc_host *host)
+{
+	/* set sampling clock selection range */
+	sd_scc_write32(host, 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, 0x00000000);
+
+	sd_scc_write32(host, SH_MOBILE_SDHI_SCC_DTCNTL,
+		SH_MOBILE_SDHI_SCC_DTCNTL_TAPEN |
+		sd_scc_read32(host, SH_MOBILE_SDHI_SCC_DTCNTL));
+
+	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, ~0x0100 &
+		sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
+
+	sd_scc_write32(host, SH_MOBILE_SDHI_SCC_CKSEL,
+		SH_MOBILE_SDHI_SCC_CKSEL_DTSEL |
+		sd_scc_read32(host, SH_MOBILE_SDHI_SCC_CKSEL));
+
+	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, 0x0100 |
+		sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
+
+	sd_scc_write32(host, SH_MOBILE_SDHI_SCC_RVSCNTL,
+		~SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN &
+		sd_scc_read32(host, SH_MOBILE_SDHI_SCC_RVSCNTL));
+
+	sd_scc_write32(host, SH_MOBILE_SDHI_SCC_DT2FF, host->scc_tappos);
+
+	/* Read TAPNUM */
+	return (sd_scc_read32(host, SH_MOBILE_SDHI_SCC_DTCNTL) >>
+		SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_SHIFT) &
+		SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_MASK;
+}
+
+static int sh_mobile_sdhi_prepare_tuning(struct tmio_mmc_host *host,
+					 unsigned long tap)
+{
+	/* Set sampling clock position */
+	sd_scc_write32(host, SH_MOBILE_SDHI_SCC_TAPSET, tap);
+
+	return 0;
+}
+
+#define SH_MOBILE_SDHI_MAX_TAP	3
+
+static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host,
+					unsigned long *tap)
+{
+	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, SH_MOBILE_SDHI_SCC_RVSREQ, 0);
+
+	/* Select SCC */
+	tap_num = (sd_scc_read32(host, SH_MOBILE_SDHI_SCC_DTCNTL) >>
+		   SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_SHIFT) &
+		SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_MASK;
+
+	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, SH_MOBILE_SDHI_SCC_TAPSET, tap_set);
+
+	/* Enable auto re-tuning */
+	sd_scc_write32(host, SH_MOBILE_SDHI_SCC_RVSCNTL,
+		SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN |
+		sd_scc_read32(host, SH_MOBILE_SDHI_SCC_RVSCNTL));
+
+	return 0;
+}
+
+static bool sh_mobile_sdhi_retuning(struct tmio_mmc_host *host)
+{
+	/* Check SCC error */
+	if (sd_scc_read32(host, SH_MOBILE_SDHI_SCC_RVSCNTL) &
+	    SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN &&
+	    sd_scc_read32(host, SH_MOBILE_SDHI_SCC_RVSREQ) &
+	    SH_MOBILE_SDHI_SCC_RVSREQ_RVSERR) {
+		/* Clear SCC error */
+		sd_scc_write32(host, SH_MOBILE_SDHI_SCC_RVSREQ, 0);
+		return true;
+	}
+	return false;
+}
+
+static void sh_mobile_sdhi_hw_reset(struct tmio_mmc_host *host)
+{
+	struct tmio_mmc_data *pdata = host->pdata;
+
+	if (pdata->flags & TMIO_MMC_HAS_UHS_SCC) {
+		/* Reset SCC */
+		sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, ~0x0100 &
+			sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
+
+		sd_scc_write32(host, SH_MOBILE_SDHI_SCC_CKSEL,
+			~SH_MOBILE_SDHI_SCC_CKSEL_DTSEL &
+			sd_scc_read32(host, SH_MOBILE_SDHI_SCC_CKSEL));
+
+		sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, 0x0100 |
+			sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
+
+		sd_scc_write32(host, SH_MOBILE_SDHI_SCC_RVSCNTL,
+			~SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN &
+			sd_scc_read32(host, SH_MOBILE_SDHI_SCC_RVSCNTL));
+
+		sd_scc_write32(host, SH_MOBILE_SDHI_SCC_RVSCNTL,
+			~SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN &
+			sd_scc_read32(host, SH_MOBILE_SDHI_SCC_RVSCNTL));
+	}
+}
+
 static int sh_mobile_sdhi_wait_idle(struct tmio_mmc_host *host)
 {
 	int timeout = 1000;
@@ -311,7 +542,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);
@@ -357,6 +588,7 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 		host->bus_shift = of_data->bus_shift;
 	}
 
+	host->set_clk_div	= sh_mobile_sdhi_set_clk_div;
 	host->dma		= dma_priv;
 	host->write16_hook	= sh_mobile_sdhi_write16_hook;
 	host->clk_enable	= sh_mobile_sdhi_clk_enable;
@@ -364,6 +596,12 @@ 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->inquiry_tuning	= sh_mobile_sdhi_inquiry_tuning;
+	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 */
@@ -403,6 +641,30 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto efree;
 
+	if (mmc_data->capabilities & MMC_CAP_UHS_SDR104) {
+		mmc_data->capabilities |= MMC_CAP_HW_RESET;
+		mmc_data->flags |= TMIO_MMC_HAS_UHS_SCC;
+	}
+
+	if (of_id && of_id->data) {
+		const struct sh_mobile_sdhi_of_data *of_data = of_id->data;
+		const struct sh_mobile_sdhi_scc *taps = of_data->taps;
+		bool hit = false;
+
+		for (i = 0; i < of_data->taps_num; i++) {
+			if (taps[i].clk == 0 ||
+			    taps[i].clk == host->mmc->f_max) {
+				host->scc_tappos = taps->tap;
+				hit = true;
+				break;
+			}
+		}
+
+		if (!hit)
+			dev_warn(&host->pdev->dev, "Unknown clock rate for SDR104 and HS200\n");
+	}
+
+	i = 0;
 	while (1) {
 		irq = platform_get_irq(pdev, i);
 		if (irq < 0)
-- 
2.1.4

Re: [PATCH/RFC 2/3] mmc: sh_mobile_sdhi: Add tuning support

[Kuninori Morimoto]

Hi Simon

Some question/comment from me

about new flags "TMIO_MMC_HAS_UHS_SCC",
you added it on [1/3] patch with this comment
"Remove unused TMIO_MMC_HAS_UHS_SCC define"
but, [1/3] patch adds it, not removed ?

> +static void sh_mobile_sdhi_hw_reset(struct tmio_mmc_host *host)
> +{
> +	struct tmio_mmc_data *pdata = host->pdata;
> +
> +	if (pdata->flags & TMIO_MMC_HAS_UHS_SCC) {
> +		/* Reset SCC */
(snip)
> +	if (mmc_data->capabilities & MMC_CAP_UHS_SDR104) {
> +		mmc_data->capabilities |= MMC_CAP_HW_RESET;
> +		mmc_data->flags |= TMIO_MMC_HAS_UHS_SCC;
> +	}

And, how about this on sh_mobile_sdhi_hw_reset() ?
We can avoid to add new flags TMIO_MMC_HAS_UHS_SCC ?

static void sh_mobile_sdhi_hw_reset(struct tmio_mmc_host *host)
{
	...

	if (pdata->capabilities & MMC_CAP_UHS_SDR104) {
		...
	}


> +static inline void sd_scc_write32(struct tmio_mmc_host *host, int addr,
> +				  u32 val)
> +{
> +	struct platform_device *pdev = host->pdev;
> +	const struct of_device_id *of_id =
> +		of_match_device(sh_mobile_sdhi_of_match, &pdev->dev);
> +	const struct sh_mobile_sdhi_of_data *of_data = of_id->data;
> +
> +	writel(val, host->ctl + of_data->scc_offset +
> +	       (addr << host->bus_shift));
> +}
(snip)
>  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),
>  };
(snip)
> +	host->set_clk_div	= sh_mobile_sdhi_set_clk_div;
>  	host->dma		= dma_priv;
>  	host->write16_hook	= sh_mobile_sdhi_write16_hook;
>  	host->clk_enable	= sh_mobile_sdhi_clk_enable;
> @@ -364,6 +596,12 @@ 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->inquiry_tuning	= sh_mobile_sdhi_inquiry_tuning;
> +	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;

You registered new callback functions on this driver,
and it is using sd_scc_read/write function, and it is based on "scc_offset".
This driver is used from not only Gen2, but also Gen1/Gen3/SH-Mobile.
But you added .scc_offset only on of_rcar_gen2_compatible.
What's happen on Gen1/Gen3/SH-Mobile ?

> +static bool sh_mobile_sdhi_inquiry_tuning(struct tmio_mmc_host *host)
> +{
> +	/* SDHI should be tuning only SDR104 */
> +	if (host->mmc->ios.timing == MMC_TIMING_UHS_SDR104)
> +		return true;
> +	else
> +		return false;
> +}

How about this ?

	return (host->mmc->ios.timing == MMC_TIMING_UHS_SDR104);

Re: [PATCH/RFC 2/3] mmc: sh_mobile_sdhi: Add tuning support

[Simon Horman]

On Tue, May 10, 2016 at 06:25:58AM +0000, Kuninori Morimoto wrote:
> 
> Hi Simon
> 
> Some question/comment from me
> 
> about new flags "TMIO_MMC_HAS_UHS_SCC",
> you added it on [1/3] patch with this comment
> "Remove unused TMIO_MMC_HAS_UHS_SCC define"
> but, [1/3] patch adds it, not removed ?

Thanks, the comment got out of sync with the code at some point.

> > +static void sh_mobile_sdhi_hw_reset(struct tmio_mmc_host *host)
> > +{
> > +	struct tmio_mmc_data *pdata = host->pdata;
> > +
> > +	if (pdata->flags & TMIO_MMC_HAS_UHS_SCC) {
> > +		/* Reset SCC */
> (snip)
> > +	if (mmc_data->capabilities & MMC_CAP_UHS_SDR104) {
> > +		mmc_data->capabilities |= MMC_CAP_HW_RESET;
> > +		mmc_data->flags |= TMIO_MMC_HAS_UHS_SCC;
> > +	}
> 
> And, how about this on sh_mobile_sdhi_hw_reset() ?
> We can avoid to add new flags TMIO_MMC_HAS_UHS_SCC ?
> 
> static void sh_mobile_sdhi_hw_reset(struct tmio_mmc_host *host)
> {
> 	...
> 
> 	if (pdata->capabilities & MMC_CAP_UHS_SDR104) {
> 		...
> 	}

That seems reasonable, I'll see if I can get it to work.

> > +static inline void sd_scc_write32(struct tmio_mmc_host *host, int addr,
> > +				  u32 val)
> > +{
> > +	struct platform_device *pdev = host->pdev;
> > +	const struct of_device_id *of_id =
> > +		of_match_device(sh_mobile_sdhi_of_match, &pdev->dev);
> > +	const struct sh_mobile_sdhi_of_data *of_data = of_id->data;
> > +
> > +	writel(val, host->ctl + of_data->scc_offset +
> > +	       (addr << host->bus_shift));
> > +}
> (snip)
> >  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),
> >  };
> (snip)
> > +	host->set_clk_div	= sh_mobile_sdhi_set_clk_div;
> >  	host->dma		= dma_priv;
> >  	host->write16_hook	= sh_mobile_sdhi_write16_hook;
> >  	host->clk_enable	= sh_mobile_sdhi_clk_enable;
> > @@ -364,6 +596,12 @@ 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->inquiry_tuning	= sh_mobile_sdhi_inquiry_tuning;
> > +	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;
> 
> You registered new callback functions on this driver,
> and it is using sd_scc_read/write function, and it is based on "scc_offset".
> This driver is used from not only Gen2, but also Gen1/Gen3/SH-Mobile.
> But you added .scc_offset only on of_rcar_gen2_compatible.
> What's happen on Gen1/Gen3/SH-Mobile ?

Probably not much good :(

I'll work on correcting that.

> > +static bool sh_mobile_sdhi_inquiry_tuning(struct tmio_mmc_host *host)
> > +{
> > +	/* SDHI should be tuning only SDR104 */
> > +	if (host->mmc->ios.timing == MMC_TIMING_UHS_SDR104)
> > +		return true;
> > +	else
> > +		return false;
> > +}
> 
> How about this ?
> 
> 	return (host->mmc->ios.timing == MMC_TIMING_UHS_SDR104);

Sure, thanks for the suggestion.

Re: [PATCH/RFC 2/3] mmc: sh_mobile_sdhi: Add tuning support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2071 bytes --]


> +struct sh_mobile_sdhi_scc {
> +	unsigned long clk;	/* clock for SDR104 */

'clk_rate' please. clk is too often used with struct clk *.

> +static void sh_mobile_sdhi_set_clk_div(struct platform_device *pdev, int state)
> +{
> +	struct mmc_host *mmc = platform_get_drvdata(pdev);
> +	struct tmio_mmc_host *host = mmc_priv(mmc);
> +
> +	if (state) {
> +		sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, ~0x0100 &
> +				sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
> +		sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, 0x00ff);
> +	}
> +}

Left over? Doesn't seemt to get used in patch 1?

> +static inline u32 sd_scc_read32(struct tmio_mmc_host *host, int addr)
> +{
> +	struct platform_device *pdev = host->pdev;
> +	const struct of_device_id *of_id =
> +		of_match_device(sh_mobile_sdhi_of_match, &pdev->dev);
> +	const struct sh_mobile_sdhi_of_data *of_data = of_id->data;
> +
> +	return readl(host->ctl + of_data->scc_offset +
> +		     (addr << host->bus_shift));
> +}
> +
> +static inline void sd_scc_write32(struct tmio_mmc_host *host, int addr,
> +				  u32 val)
> +{
> +	struct platform_device *pdev = host->pdev;
> +	const struct of_device_id *of_id =
> +		of_match_device(sh_mobile_sdhi_of_match, &pdev->dev);
> +	const struct sh_mobile_sdhi_of_data *of_data = of_id->data;
> +
> +	writel(val, host->ctl + of_data->scc_offset +
> +	       (addr << host->bus_shift));

It probably makes sense to store the SCC base pointer somewhere to
prevent all these lookups with every read/write.

> +static bool sh_mobile_sdhi_inquiry_tuning(struct tmio_mmc_host *host)
> +{
> +	/* SDHI should be tuning only SDR104 */
> +	if (host->mmc->ios.timing == MMC_TIMING_UHS_SDR104)
> +		return true;
> +	else
> +		return false;
> +}

Really needed? See patch 1.

>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -357,6 +588,7 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
>  		host->bus_shift = of_data->bus_shift;
>  	}
>  
> +	host->set_clk_div	= sh_mobile_sdhi_set_clk_div;

Left over?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH/RFC 3/3] 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.1.4

Re: [PATCH/RFC 3/3] ARM: dts: r8a7790: lager: Enable UHS-I SDR-104

[Magnus Damm]

On Tue, May 10, 2016 at 2:52 PM, Simon Horman
<horms+renesas@verge.net.au> wrote:
> 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";

Hi Simon,

Thanks for this - interesting to see!! From what I can tell this code
is targeting r8a7790 Lager, and based on the data sheet and the DTS
there are four SDHI channels for the r8a7790 SoC. All R-Car Gen2 SDHI
channels are not identical, so on r8a7790 it looks like SDHI0 and
SDHI1 have extended capabilities for the clocks (and probably support
SDR104) while SDHI2 and SDHI3 do not have this hardware feature. How
is this difference handled today? In my mind it would make sense to
have different compat strings, but I think we differentiate with
resource size today?

So with this patches I can see that you enable SDR104 on SDHI0 on
Lager which makes sense, but is SDHI2 support still OK?

Cheers,

/ magnus

Re: [PATCH/RFC 3/3] ARM: dts: r8a7790: lager: Enable UHS-I SDR-104

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 216 bytes --]


> SDR104) while SDHI2 and SDHI3 do not have this hardware feature. How
> is this difference handled today?

We read out the version register. (That reminds me I have a
documentation patch for that lying around...)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH/RFC 3/3] ARM: dts: r8a7790: lager: Enable UHS-I SDR-104

[Simon Horman]

On Tue, May 10, 2016 at 06:13:55PM +0900, Magnus Damm wrote:
> On Tue, May 10, 2016 at 2:52 PM, Simon Horman
> <horms+renesas@verge.net.au> wrote:
> > 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";
> 
> Hi Simon,
> 
> Thanks for this - interesting to see!! From what I can tell this code
> is targeting r8a7790 Lager, and based on the data sheet and the DTS
> there are four SDHI channels for the r8a7790 SoC. All R-Car Gen2 SDHI
> channels are not identical, so on r8a7790 it looks like SDHI0 and
> SDHI1 have extended capabilities for the clocks (and probably support
> SDR104) while SDHI2 and SDHI3 do not have this hardware feature. How
> is this difference handled today? In my mind it would make sense to
> have different compat strings, but I think we differentiate with
> resource size today?

Probably I am missing the point somehow but my understanding is that that
with this and other recent changes these differences are described by DT
properties rather than the compatibility string. In particular the presence
or absence of sd-uhs-sdr50 and sd-uhs-sdr104, and the value of
max-frequency.

Now you mention this I do wonder if sd-uhs-sdr50 and sd-uhs-sdr104 are
SoC properties and thus should be present in r8a7790.dtsi rather than
r8a7790-lager.dts.

> So with this patches I can see that you enable SDR104 on SDHI0 on
> Lager which makes sense, but is SDHI2 support still OK?

I will double check but I think so.

RE: [PATCH/RFC 0/3] UHS-I SDR-104 support for sh_mobile_sdhi

[Yoshihiro Shimoda]

Hi Simon-san,

> From: Simon Horman
> Sent: Tuesday, May 10, 2016 2:52 PM
> 
> 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 a merge of:
> * The next branch of the mmc tree
> * The renesas-next-20160427v2-v4.6-rc1 tag of the renesas tree
> * The sh-pfc-for-v4.7-tag1 tag of the renesas-drivers tree
> 
> The first two 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 branch
> of the renesas tree.

Thank you for the patch set!
I tried to the patch using the topic branch.

> A description of testing performed follows:
> 
> * Environment
> 
>   SoC/Board: r8a7790/Lager
>   SanDisk Card: SanDisk Ultra 64Gb microSDXC UHS-1
>                 Identifier on Packaging: SDSQUNC-064G-GN6MA
>   Samsung Card: Samsung 32Gb microSDHC Card Class 10 UHS-1 U3
>                 ModelCode: MB-MG32EA/FFP

I used r8a7790/Lager and the following card:

   SanDisk Card: SanDisk Extreme Pro 32Gb microSDHC U3
   (Sorry I don't have the packaging because I borrowed it from my colleague.)

< Result >
- It seems worked correctly.
- I tried 3 Lager boards:
 - One of board (No.701 / ES2.0) sometimes works correctly.
  - " mmc1: tuning execution failed: -110" appeared.
 - Other 2 boards (No.465 / ES2.0 and No.322 / ES3.0) always works correctly.

Perhaps the issue is related to hardware problem.

< Detail >
If SDR104 works:
[   19.348032] mmc1: new ultra high speed SDR104 SDHC card at address aaaa
[   19.355352] mmcblk1: mmc1:aaaa SP32G 29.7 GiB 
[   19.370433]  mmcblk1: p1

root@192:~# dd bs=1M count=512 iflag=direct if=/dev/mmcblk1 of=/dev/null
512+0 records in
512+0 records out
536870912 bytes (537 MB) copied, 11.1761 s, 48.0 MB/s

If I remove sdr104 property and worked as SDR50:
[   15.050218] mmc1: new ultra high speed SDR50 SDHC card at address 
aaaa                                                                            
[   15.057282] mmcblk1: mmc1:aaaa SP32G 29.7 GiB                                
[   15.071917]  mmcblk1: p1

root@192:~# dd bs=1M count=512 iflag=direct if=/dev/mmcblk1 of=/dev/null        
512+0 records in                                                                
512+0 records out                                                               
536870912 bytes (537 MB) copied, 15.514 s, 34.6 MB/s

< log of the issue (tuning failed) >
The following message appeared when I connected the card and about 5 seconds later.
[   21.962403] mmc1: tuning execution failed: -110
[   21.967002] mmc1: error -110 whilst initialising SD card
[   21.973813] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
[   21.981875] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
[   21.989978] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
[   21.998036] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
[   22.006114] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
[   27.013768] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
[   27.035294] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
[   27.053749] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
[   27.074091] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
[   27.082471] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
[   27.090824] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
[   27.114029] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
[   27.134033] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
[   27.142345] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
[   32.152388] sh_mobile_sdhi ee100000.sd: timeout waiting for hardware interrupt (CMD52)
[   37.202394] sh_mobile_sdhi ee100000.sd: timeout waiting for hardware interrupt (CMD52)

Did you see such a message on your board?

Best regards,
Yoshihiro Shimoda

> * Wit these patches (topic/sdr104 branch)
> 
>   # mount -t debugfs none /sys/kernel/debug
>   # cat /sys/kernel/debug/mmc1/ios
> 
>   clock:          195000000 Hz
>   vdd:            21 (3.3 ~ 3.4 V)
>   bus mode:       2 (push-pull)
>   chip select:    0 (don't care)
>   power mode:     2 (on)
>   bus width:      2 (4 bits)
>   timing spec:    6 (sd uhs SDR104)
>   signal voltage: 1 (1.80 V)
>   driver type:    0 (driver type B)
> 
>   SanDisk Card:
>   # dd bs=1M count=512 iflag=direct if=/dev/mmcblk1 of=/dev/null
>   536870912 bytes (537 MB) copied, 13.6713 s, 39.3 MB/s
> 
>   Samsung Card:
>   # dd bs=1M count=512 iflag=direct if=/dev/mmcblk1 of=/dev/null
>   536870912 bytes (537 MB) copied, 16.4952 s, 32.5 MB/s
> 
> 
> * Without these patches (base of topic/sdr104 branch)
> 
>   # mount -t debugfs none /sys/kernel/debug
>   # cat /sys/kernel/debug/mmc1/ios
> 
>   clock:          100000000 Hz
>   vdd:            21 (3.3 ~ 3.4 V)
>   bus mode:       2 (push-pull)
>   chip select:    0 (don't care)
>   power mode:     2 (on)
>   bus width:      2 (4 bits)
>   timing spec:    5 (sd uhs SDR50)
>   signal voltage: 1 (1.80 V)
>   driver type:    0 (driver type B)
> 
>   SanDisk Card:
>   # dd bs=1M count=512 iflag=direct if=/dev/mmcblk1 of=/dev/null
>   536870912 bytes (537 MB) copied, 17.7253 s, 30.3 MB/s
> 
>   Samsung Card:
>   # dd bs=1M count=512 iflag=direct if=/dev/mmcblk1 of=/dev/null
>   536870912 bytes (537 MB) copied, 20.7866 s, 25.8 MB/s
> 
> 
> 
> Ai Kyuse (2):
>   mmc: tmio: Add tuning support
>   mmc: sh_mobile_sdhi: Add tuning support
> 
> Simon Horman (1):
>   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         |  10 ++
>  drivers/mmc/host/tmio_mmc_pio.c     | 249 ++++++++++++++++++++++++++++++++--
>  include/linux/mfd/tmio.h            |   3 +
>  5 files changed, 518 insertions(+), 9 deletions(-)
> 
> --
> 2.1.4

Re: [PATCH/RFC 0/3] UHS-I SDR-104 support for sh_mobile_sdhi

[Geert Uytterhoeven]

Hi Shimoda-san,

On Thu, May 12, 2016 at 8:30 AM, Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> - I tried 3 Lager boards:
>  - One of board (No.701 / ES2.0) sometimes works correctly.
>   - " mmc1: tuning execution failed: -110" appeared.
>  - Other 2 boards (No.465 / ES2.0 and No.322 / ES3.0) always works correctly.
>
> Perhaps the issue is related to hardware problem.

Do they have the same U-Boot version?
Does the failing one have a newer U-Boot that disables unused MSTP clocks?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH/RFC 0/3] UHS-I SDR-104 support for sh_mobile_sdhi

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven
> Sent: Thursday, May 12, 2016 3:46 PM
> 
> Hi Shimoda-san,
> 
> On Thu, May 12, 2016 at 8:30 AM, Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > - I tried 3 Lager boards:
> >  - One of board (No.701 / ES2.0) sometimes works correctly.
> >   - " mmc1: tuning execution failed: -110" appeared.
> >  - Other 2 boards (No.465 / ES2.0 and No.322 / ES3.0) always works correctly.
> >
> > Perhaps the issue is related to hardware problem.
> 
> Do they have the same U-Boot version?
> Does the failing one have a newer U-Boot that disables unused MSTP clocks?

Thank you for the comment! The No.701 was difference U-Boot version.
So, I wrote the same U-Boot version into the No.701 board, and then
SDR104 works correctly on the board.

Best regards,
Yoshihiro Shimoda

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH/RFC 0/3] UHS-I SDR-104 support for sh_mobile_sdhi

[Geert Uytterhoeven]

Hi Shimoda-san,

On Thu, May 12, 2016 at 9:45 AM, Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
>> From: Geert Uytterhoeven
>> Sent: Thursday, May 12, 2016 3:46 PM
>>
>> On Thu, May 12, 2016 at 8:30 AM, Yoshihiro Shimoda
>> <yoshihiro.shimoda.uh@renesas.com> wrote:
>> > - I tried 3 Lager boards:
>> >  - One of board (No.701 / ES2.0) sometimes works correctly.
>> >   - " mmc1: tuning execution failed: -110" appeared.
>> >  - Other 2 boards (No.465 / ES2.0 and No.322 / ES3.0) always works correctly.
>> >
>> > Perhaps the issue is related to hardware problem.
>>
>> Do they have the same U-Boot version?
>> Does the failing one have a newer U-Boot that disables unused MSTP clocks?
>
> Thank you for the comment! The No.701 was difference U-Boot version.
> So, I wrote the same U-Boot version into the No.701 board, and then
> SDR104 works correctly on the board.

What are the U-Boot versions?

This is still a kernel bug that needs to be fixed...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH/RFC 0/3] UHS-I SDR-104 support for sh_mobile_sdhi

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven
> Sent: Thursday, May 12, 2016 4:47 PM
> 
> Hi Shimoda-san,
> 
> On Thu, May 12, 2016 at 9:45 AM, Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> >> From: Geert Uytterhoeven
> >> Sent: Thursday, May 12, 2016 3:46 PM
> >>
> >> On Thu, May 12, 2016 at 8:30 AM, Yoshihiro Shimoda
> >> <yoshihiro.shimoda.uh@renesas.com> wrote:
> >> > - I tried 3 Lager boards:
> >> >  - One of board (No.701 / ES2.0) sometimes works correctly.
> >> >   - " mmc1: tuning execution failed: -110" appeared.
> >> >  - Other 2 boards (No.465 / ES2.0 and No.322 / ES3.0) always works correctly.
> >> >
> >> > Perhaps the issue is related to hardware problem.
> >>
> >> Do they have the same U-Boot version?
> >> Does the failing one have a newer U-Boot that disables unused MSTP clocks?
> >
> > Thank you for the comment! The No.701 was difference U-Boot version.
> > So, I wrote the same U-Boot version into the No.701 board, and then
> > SDR104 works correctly on the board.
> 
> What are the U-Boot versions?

"U-Boot 2013.01.01-gcb82c56": SDR104 works fine.
http://git.denx.de/?p=u-boot/u-boot-sh.git;a=log;h=refs/heads/renesas/bsp/rcar-gen2-1.9.4

"U-Boot 2015.04-dirty": Sometimes SDR104 works fine.
Sorry, I don't know actual commit id.

> This is still a kernel bug that needs to be fixed...

I think so...

Best regards,
Yoshihiro Shimoda

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH/RFC 0/3] UHS-I SDR-104 support for sh_mobile_sdhi

[Geert Uytterhoeven]

Hi Shimoda-sanm

On Thu, May 12, 2016 at 10:09 AM, Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
>> From: Geert Uytterhoeven
>> Sent: Thursday, May 12, 2016 4:47 PM
>> On Thu, May 12, 2016 at 9:45 AM, Yoshihiro Shimoda
>> <yoshihiro.shimoda.uh@renesas.com> wrote:
>> >> From: Geert Uytterhoeven
>> >> Sent: Thursday, May 12, 2016 3:46 PM
>> >>
>> >> On Thu, May 12, 2016 at 8:30 AM, Yoshihiro Shimoda
>> >> <yoshihiro.shimoda.uh@renesas.com> wrote:
>> >> > - I tried 3 Lager boards:
>> >> >  - One of board (No.701 / ES2.0) sometimes works correctly.
>> >> >   - " mmc1: tuning execution failed: -110" appeared.
>> >> >  - Other 2 boards (No.465 / ES2.0 and No.322 / ES3.0) always works correctly.
>> >> >
>> >> > Perhaps the issue is related to hardware problem.
>> >>
>> >> Do they have the same U-Boot version?
>> >> Does the failing one have a newer U-Boot that disables unused MSTP clocks?
>> >
>> > Thank you for the comment! The No.701 was difference U-Boot version.
>> > So, I wrote the same U-Boot version into the No.701 board, and then
>> > SDR104 works correctly on the board.
>>
>> What are the U-Boot versions?
>
> "U-Boot 2013.01.01-gcb82c56": SDR104 works fine.
> http://git.denx.de/?p=u-boot/u-boot-sh.git;a=log;h=refs/heads/renesas/bsp/rcar-gen2-1.9.4
>
> "U-Boot 2015.04-dirty": Sometimes SDR104 works fine.
> Sorry, I don't know actual commit id.

v2015.04 is good enough the confirm my suspicion.

>> This is still a kernel bug that needs to be fixed...
>
> I think so...

I suppose it fails again if you merge topic/renesas-debug from my
renesas-drivers
git repo, as that will disable the unused MSTP clocks again on boot?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH/RFC 0/3] UHS-I SDR-104 support for sh_mobile_sdhi

[Geert Uytterhoeven]

Hi Shimoda-san,

On Thu, May 12, 2016 at 11:26 AM, Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
>> From: Geert Uytterhoeven
>> Sent: Thursday, May 12, 2016 5:32 PM
>> On Thu, May 12, 2016 at 10:09 AM, Yoshihiro Shimoda
>> <yoshihiro.shimoda.uh@renesas.com> wrote:
>> >> From: Geert Uytterhoeven
>> >> Sent: Thursday, May 12, 2016 4:47 PM
>> >> On Thu, May 12, 2016 at 9:45 AM, Yoshihiro Shimoda
>> >> <yoshihiro.shimoda.uh@renesas.com> wrote:
>> >> >> From: Geert Uytterhoeven
>> >> >> Sent: Thursday, May 12, 2016 3:46 PM
>> >> >>
>> >> >> On Thu, May 12, 2016 at 8:30 AM, Yoshihiro Shimoda
>> >> >> <yoshihiro.shimoda.uh@renesas.com> wrote:
>> >> >> > - I tried 3 Lager boards:
>> >> >> >  - One of board (No.701 / ES2.0) sometimes works correctly.
>> >> >> >   - " mmc1: tuning execution failed: -110" appeared.
>> >> >> >  - Other 2 boards (No.465 / ES2.0 and No.322 / ES3.0) always works correctly.
>> >> >> >
>> >> >> > Perhaps the issue is related to hardware problem.
>> >> >>
>> >> >> Do they have the same U-Boot version?
>> >> >> Does the failing one have a newer U-Boot that disables unused MSTP clocks?
>> >> >
>> >> > Thank you for the comment! The No.701 was difference U-Boot version.
>> >> > So, I wrote the same U-Boot version into the No.701 board, and then
>> >> > SDR104 works correctly on the board.
>> >>
>> >> What are the U-Boot versions?
>> >
>> > "U-Boot 2013.01.01-gcb82c56": SDR104 works fine.
>> > http://git.denx.de/?p=u-boot/u-boot-sh.git;a=log;h=refs/heads/renesas/bsp/rcar-gen2-1.9.4
>> >
>> > "U-Boot 2015.04-dirty": Sometimes SDR104 works fine.
>> > Sorry, I don't know actual commit id.
>>
>> v2015.04 is good enough the confirm my suspicion.
>>
>> >> This is still a kernel bug that needs to be fixed...
>> >
>> > I think so...
>>
>> I suppose it fails again if you merge topic/renesas-debug from my
>> renesas-drivers
>> git repo, as that will disable the unused MSTP clocks again on boot?
>
> I tried the topic/renesas-debug from your renesas-drivers git repo.
> However, the issue still appeared.

That's what I expected...

> For your reference, I changed the pr_debug() to pr_info() in clk-mstp.c
> and I pasted the kernel log to the end of this email.

Good, so we know which module clocks are enabled/disabled...

> [    0.024351] Disabling MSTP clocks for the System Core
> [    0.024364]   SMSTPCR0: *0xe6150130 |= 0x00640801
> [    0.024377]   SMSTPCR1: *0xe6150134 |= 0xdb6e9bdf
> [    0.024390]   SMSTPCR2: *0xe6150138 |= 0x300da1fc
> [    0.024402]   SMSTPCR3: *0xe615013c |= 0xf08cfc31
> [    0.024415]   SMSTPCR4: *0xe6150140 |= 0x80000004
> [    0.024427]   SMSTPCR5: *0xe6150144 |= 0x44c00046
> [    0.024439]   SMSTPCR6: *0xe615014c |= 0x07d30718
> [    0.024452]   SMSTPCR7: *0xe6150990 |= 0x01f0ff84
> [    0.024464]   SMSTPCR8: *0xe6150994 |= 0xf5d79fcf
> [    0.024476]   SMSTPCR9: *0xe6150998 |= 0xfffeffe0
> [    0.024489]   SMSTPCR10: *0xe615099c |= 0x00000000
> [    0.024501] Disabling MSTP clocks for the Realtime Core
> [    0.024513]   RMSTPCR0: *0xe6150110 |= 0x00640801
> [    0.024526]   RMSTPCR1: *0xe6150114 |= 0xdb6e9bdf
> [    0.024538]   RMSTPCR2: *0xe6150118 |= 0x300da1fc
> [    0.024550]   RMSTPCR3: *0xe615011c |= 0xf08cfc31
> [    0.024562]   RMSTPCR4: *0xe6150120 |= 0x80000184
> [    0.024574]   RMSTPCR5: *0xe6150124 |= 0x44c00046
> [    0.024586]   RMSTPCR6: *0xe615012c |= 0x07f30718
> [    0.024599]   RMSTPCR7: *0xe6150980 |= 0x01f0ff84
> [    0.024611]   RMSTPCR8: *0xe6150984 |= 0xf5d79fcf
> [    0.024623]   RMSTPCR9: *0xe6150988 |= 0xfffeffe0
> [    0.024635]   RMSTPCR10: *0xe615098c |= 0x00000000

The above disabled all unused MSTP clocks...

> [    0.024687] INTC_SYS            : S .
> [    0.024700] IRQC                : S .
> [    0.024730] SCIF0               : S .

... after which only the 3 above are still enabled.

> [    3.884952] MSTP sdhi0 ON

The sdhi0 MSTP clock is now enabled (and never disabled later)...

> [    4.023731] sh_mobile_sdhi ee100000.sd: mmc1 base at 0xee100000 max clock rate 195 MHz
> [    4.032324] sh_mobile_sdhi ee140000.sd: Got CD GPIO

> root@192:~# [   26.983564] mmc1: tuning execution failed: -110
> [   26.988177] mmc1: error -110 whilst initialising SD card
> [   26.995011] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
> [   27.003101] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
> [   27.011225] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
> [   27.019313] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
> [   27.027418] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
> [   32.034962] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
> [   32.056492] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
> [   32.074949] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
> [   32.095257] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
> [   32.103655] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
> [   32.112020] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
> [   32.135220] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
> [   32.155239] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
> [   32.163633] sh_mobile_sdhi ee100000.sd: timeout waiting for SD bus idle
> [   37.173569] sh_mobile_sdhi ee100000.sd: timeout waiting for hardware interrupt (CMD52)
> [   42.223562] sh_mobile_sdhi ee100000.sd: timeout waiting for hardware interrupt (CMD52)

... still it failed.

As the sdhi0 MSTP clock is still enabled, there must be an unknown clock that's
needed for proper operation, but that's currently not controlled by software.

With topic/renesas-debug, you can enable/disable module clocks from the
shell, e.g.

              echo sdhi0=on > /proc/mstp
              echo sdhi0=0 > /proc/mstp
              echo 314=off > /proc/mstp

or control which clocks are disabled at boot time by modifying the
SMSTPx_DISABLE macros.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH/RFC 0/3] UHS-I SDR-104 support for sh_mobile_sdhi

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 207 bytes --]


> As the sdhi0 MSTP clock is still enabled, there must be an unknown clock that's
> needed for proper operation, but that's currently not controlled by software.

I'd guess the SCC needs a seperate clock?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH/RFC 0/3] UHS-I SDR-104 support for sh_mobile_sdhi

[Geert Uytterhoeven]

Hi Wolfram,

On Thu, May 12, 2016 at 2:41 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> As the sdhi0 MSTP clock is still enabled, there must be an unknown clock that's
>> needed for proper operation, but that's currently not controlled by software.
>
> I'd guess the SCC needs a seperate clock?

Probably, but this is not documented in the datasheet.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH/RFC 0/3] UHS-I SDR-104 support for sh_mobile_sdhi

[Simon Horman]

On Thu, May 12, 2016 at 06:30:04AM +0000, Yoshihiro Shimoda wrote:
> Hi Simon-san,
> 
> > From: Simon Horman
> > Sent: Tuesday, May 10, 2016 2:52 PM
> > 
> > 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 a merge of:
> > * The next branch of the mmc tree
> > * The renesas-next-20160427v2-v4.6-rc1 tag of the renesas tree
> > * The sh-pfc-for-v4.7-tag1 tag of the renesas-drivers tree
> > 
> > The first two 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 branch
> > of the renesas tree.
> 
> Thank you for the patch set!
> I tried to the patch using the topic branch.
> 
> > A description of testing performed follows:
> > 
> > * Environment
> > 
> >   SoC/Board: r8a7790/Lager
> >   SanDisk Card: SanDisk Ultra 64Gb microSDXC UHS-1
> >                 Identifier on Packaging: SDSQUNC-064G-GN6MA
> >   Samsung Card: Samsung 32Gb microSDHC Card Class 10 UHS-1 U3
> >                 ModelCode: MB-MG32EA/FFP
> 
> I used r8a7790/Lager and the following card:
> 
>    SanDisk Card: SanDisk Extreme Pro 32Gb microSDHC U3
>    (Sorry I don't have the packaging because I borrowed it from my colleague.)

My car has some rather difficult to read information printed on the back.
We can compare that if you think it is useful.

> < Result >
> - It seems worked correctly.
> - I tried 3 Lager boards:
>  - One of board (No.701 / ES2.0) sometimes works correctly.
>   - " mmc1: tuning execution failed: -110" appeared.
>  - Other 2 boards (No.465 / ES2.0 and No.322 / ES3.0) always works correctly.
> 
> Perhaps the issue is related to hardware problem.

[...]

Some discussion of that followed. I'll leave that as a separate sub-thread.