[PATCH v2 0/9] r8a7790: add UHS-I (SDR50) support to Lager

[PATCH v2 0/9] r8a7790: add UHS-I (SDR50) support to Lager

[Wolfram Sang]

So, here is v2 of the series adding SDR50 support to the Renesas Lager board.
Change since v1:

* rebased to renesas-drivers based on v4.6-rc1
* dropped pinctrl patch which was already picked up
* directly populate start_signal_voltage_switch() in mmc_ops
  instead of using a wrapper function
* use generic DT pinctrl bindings instead of vendor specific ones
  (e.g. "groups" instead of "renesas,groups")

The RFC series worked fine with my Transcend card, but failed to switch
voltages on a SanDisk and Samsung card. This bug hunting resulted in patches
4-6 newly added: The clock really has to be disabled on ios->clock == 0,
setting frequency to 0Hz doesn't work. I wonder if this isn't true for some
more controllers? Is this known already?

My "copy-large-files-around"-setup showed now 30MB/s while it had 19MB/s
without SDR50. Frankly, I hoped for a little more, but let's start with this
initial support and do the tuning incrementally I'd say.

Patches 1-7 should go via Ulf. After those patches went in, Simon can take 8+9
to tie it all together.

Please comment, test, apply...

Thanks,

   Wolfram


Ben Hutchings (3):
  mmc: tmio, sh_mobile_sdhi: Pass tmio_mmc_host ptr to
    clk_{enable,disable} ops
  mmc: tmio, sh_mobile_sdhi: Add support for variable input clock
    frequency
  ARM: shmobile: r8a7790: Set maximum frequencies for SDHI clocks

Wolfram Sang (6):
  mmc: tmio: Add UHS-I mode support
  mmc: tmio: always start clock after frequency calculation
  mmc: tmio: stop clock when 0Hz is requested
  mmc: host: add note that set_ios needs to handle 0Hz properly
  mmc: sh_mobile_sdhi: Add UHS-I mode support
  ARM: shmobile: r8a7790: lager: Enable UHS-I SDR-50

 Documentation/devicetree/bindings/mmc/tmio_mmc.txt |   3 +
 arch/arm/boot/dts/r8a7790-lager.dts                |  22 +++-
 arch/arm/boot/dts/r8a7790.dtsi                     |   4 +
 drivers/mmc/host/sh_mobile_sdhi.c                  | 116 +++++++++++++++++++--
 drivers/mmc/host/tmio_mmc.h                        |   8 +-
 drivers/mmc/host/tmio_mmc_pio.c                    | 102 +++++++++---------
 include/linux/mmc/host.h                           |  31 ++++--
 include/linux/mmc/tmio.h                           |   2 +
 8 files changed, 218 insertions(+), 70 deletions(-)

-- 
2.7.0

[PATCH v2 1/9] mmc: tmio, sh_mobile_sdhi: Pass tmio_mmc_host ptr to clk_{enable,disable} ops

[Wolfram Sang]

From: Ben Hutchings <ben.hutchings@codethink.co.uk>

Change the clk_enable operation to take a pointer to the struct
tmio_mmc_host and have it set f_max.  For consistency, also change the
clk_disable operation to take a pointer to struct tmio_mmc_host.

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/sh_mobile_sdhi.c | 12 +++++-------
 drivers/mmc/host/tmio_mmc.h       |  4 ++--
 drivers/mmc/host/tmio_mmc_pio.c   |  4 ++--
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 9aa147959276d0..55849350202b7d 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -131,16 +131,15 @@ static void sh_mobile_sdhi_sdbuf_width(struct tmio_mmc_host *host, int width)
 	sd_ctrl_write16(host, EXT_ACC, val);
 }
 
-static int sh_mobile_sdhi_clk_enable(struct platform_device *pdev, unsigned int *f)
+static int sh_mobile_sdhi_clk_enable(struct tmio_mmc_host *host)
 {
-	struct mmc_host *mmc = platform_get_drvdata(pdev);
-	struct tmio_mmc_host *host = mmc_priv(mmc);
+	struct mmc_host *mmc = host->mmc;
 	struct sh_mobile_sdhi *priv = host_to_priv(host);
 	int ret = clk_prepare_enable(priv->clk);
 	if (ret < 0)
 		return ret;
 
-	*f = clk_get_rate(priv->clk);
+	mmc->f_max = clk_get_rate(priv->clk);
 
 	/* enable 16bit data access on SDBUF as default */
 	sh_mobile_sdhi_sdbuf_width(host, 16);
@@ -148,11 +147,10 @@ static int sh_mobile_sdhi_clk_enable(struct platform_device *pdev, unsigned int
 	return 0;
 }
 
-static void sh_mobile_sdhi_clk_disable(struct platform_device *pdev)
+static void sh_mobile_sdhi_clk_disable(struct tmio_mmc_host *host)
 {
-	struct mmc_host *mmc = platform_get_drvdata(pdev);
-	struct tmio_mmc_host *host = mmc_priv(mmc);
 	struct sh_mobile_sdhi *priv = host_to_priv(host);
+
 	clk_disable_unprepare(priv->clk);
 }
 
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 4a597f5a53e20f..68fd8d791358c1 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -95,8 +95,8 @@ struct tmio_mmc_host {
 	bool			sdio_irq_enabled;
 
 	int (*write16_hook)(struct tmio_mmc_host *host, int addr);
-	int (*clk_enable)(struct platform_device *pdev, unsigned int *f);
-	void (*clk_disable)(struct platform_device *pdev);
+	int (*clk_enable)(struct tmio_mmc_host *host);
+	void (*clk_disable)(struct tmio_mmc_host *host);
 	int (*multi_io_quirk)(struct mmc_card *card,
 			      unsigned int direction, int blk_size);
 };
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index 03f6e74c190691..d1160156678ade 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -845,7 +845,7 @@ static int tmio_mmc_clk_update(struct tmio_mmc_host *host)
 	if (!host->clk_enable)
 		return -ENOTSUPP;
 
-	ret = host->clk_enable(host->pdev, &mmc->f_max);
+	ret = host->clk_enable(host);
 	if (!ret)
 		mmc->f_min = mmc->f_max / 512;
 
@@ -1251,7 +1251,7 @@ int tmio_mmc_host_runtime_suspend(struct device *dev)
 		tmio_mmc_clk_stop(host);
 
 	if (host->clk_disable)
-		host->clk_disable(host->pdev);
+		host->clk_disable(host);
 
 	return 0;
 }
-- 
2.7.0

[PATCH v2 2/9] mmc: tmio, sh_mobile_sdhi: Add support for variable input clock frequency

[Wolfram Sang]

From: Ben Hutchings <ben.hutchings@codethink.co.uk>

Currently tmio_mmc assumes that the input clock frequency is fixed and
only its own clock divider can be changed.  This is not true in the
case of sh_mobile_sdhi; we can use the clock API to change it.

In tmio_mmc:
- Delegate setting of f_min from tmio to the clk_enable operation (if
  implemented), as it can be smaller than f_max / 512
- Add an optional clk_update operation called from tmio_mmc_set_clock()
  that updates the input clock frequency
- Rename tmio_mmc_clk_update() to tmio_mmc_clk_enable(), to avoid
  confusion with the clk_update operation

In sh_mobile_sdhi:
- Make the setting of f_max conditional; it should be set through the
  max-frequency property in the device tree in future
- Set f_min based on the input clock's minimum frequency
- Implement the clk_update operation, selecting the best input clock
  frequency for the bus frequency that's wanted

sh_mobile_sdhi_clk_update() is loosely based on Kuninori Morimoto's work
in sh_mmcif.

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/sh_mobile_sdhi.c | 56 +++++++++++++++++++++++++++++++++++++--
 drivers/mmc/host/tmio_mmc.h       |  2 ++
 drivers/mmc/host/tmio_mmc_pio.c   | 24 +++++++----------
 3 files changed, 66 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 55849350202b7d..8fd1d6b29190b6 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -139,7 +139,20 @@ static int sh_mobile_sdhi_clk_enable(struct tmio_mmc_host *host)
 	if (ret < 0)
 		return ret;
 
-	mmc->f_max = clk_get_rate(priv->clk);
+	/*
+	 * The clock driver may not know what maximum frequency
+	 * actually works, so it should be set with the max-frequency
+	 * property which will already have been read to f_max.  If it
+	 * was missing, assume the current frequency is the maximum.
+	 */
+	if (!mmc->f_max)
+		mmc->f_max = clk_get_rate(priv->clk);
+
+	/*
+	 * Minimum frequency is the minimum input clock frequency
+	 * divided by our maximum divider.
+	 */
+	mmc->f_min = max(clk_round_rate(priv->clk, 1) / 512, 1L);
 
 	/* enable 16bit data access on SDBUF as default */
 	sh_mobile_sdhi_sdbuf_width(host, 16);
@@ -147,6 +160,44 @@ static int sh_mobile_sdhi_clk_enable(struct tmio_mmc_host *host)
 	return 0;
 }
 
+static unsigned int sh_mobile_sdhi_clk_update(struct tmio_mmc_host *host,
+					      unsigned int new_clock)
+{
+	struct sh_mobile_sdhi *priv = host_to_priv(host);
+	unsigned int freq, best_freq, diff_min, diff;
+	int i;
+
+	diff_min = ~0;
+	best_freq = 0;
+
+	/*
+	 * We want the bus clock to be as close as possible to, but no
+	 * greater than, new_clock.  As we can divide by 1 << i for
+	 * any i in [0, 9] we want the input clock to be as close as
+	 * possible, but no greater than, new_clock << i.
+	 */
+	for (i = min(9, ilog2(UINT_MAX / new_clock)); i >= 0; i--) {
+		freq = clk_round_rate(priv->clk, new_clock << i);
+		if (freq > (new_clock << i)) {
+			/* Too fast; look for a slightly slower option */
+			freq = clk_round_rate(priv->clk,
+					      (new_clock << i) / 4 * 3);
+			if (freq > (new_clock << i))
+				continue;
+		}
+
+		diff = new_clock - (freq >> i);
+		if (diff <= diff_min) {
+			best_freq = freq;
+			diff_min = diff;
+		}
+	}
+
+	clk_set_rate(priv->clk, best_freq);
+
+	return best_freq;
+}
+
 static void sh_mobile_sdhi_clk_disable(struct tmio_mmc_host *host)
 {
 	struct sh_mobile_sdhi *priv = host_to_priv(host);
@@ -265,6 +316,7 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 	host->dma		= dma_priv;
 	host->write16_hook	= sh_mobile_sdhi_write16_hook;
 	host->clk_enable	= sh_mobile_sdhi_clk_enable;
+	host->clk_update	= sh_mobile_sdhi_clk_update;
 	host->clk_disable	= sh_mobile_sdhi_clk_disable;
 	host->multi_io_quirk	= sh_mobile_sdhi_multi_io_quirk;
 
@@ -362,7 +414,7 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 		}
 	}
 
-	dev_info(&pdev->dev, "%s base at 0x%08lx clock rate %u MHz\n",
+	dev_info(&pdev->dev, "%s base at 0x%08lx max clock rate %u MHz\n",
 		 mmc_hostname(host->mmc), (unsigned long)
 		 (platform_get_resource(pdev, IORESOURCE_MEM, 0)->start),
 		 host->mmc->f_max / 1000000);
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 68fd8d791358c1..b44b5890290622 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -96,6 +96,8 @@ struct tmio_mmc_host {
 
 	int (*write16_hook)(struct tmio_mmc_host *host, int addr);
 	int (*clk_enable)(struct tmio_mmc_host *host);
+	unsigned int (*clk_update)(struct tmio_mmc_host *host,
+				   unsigned int new_clock);
 	void (*clk_disable)(struct tmio_mmc_host *host);
 	int (*multi_io_quirk)(struct mmc_card *card,
 			      unsigned int direction, int blk_size);
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index d1160156678ade..ae81b34f17a5a5 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -160,9 +160,12 @@ static void tmio_mmc_set_clock(struct tmio_mmc_host *host,
 	u32 clk = 0, clock;
 
 	if (new_clock) {
-		for (clock = host->mmc->f_min, clk = 0x80000080;
-		     new_clock >= (clock << 1);
-		     clk >>= 1)
+		if (host->clk_update)
+			clock = host->clk_update(host, new_clock) / 512;
+		else
+			clock = host->mmc->f_min;
+
+		for (clk = 0x80000080; new_clock >= (clock << 1); clk >>= 1)
 			clock <<= 1;
 
 		/* 1/1 clock is option */
@@ -837,19 +840,12 @@ fail:
 	pm_runtime_put_autosuspend(mmc_dev(mmc));
 }
 
-static int tmio_mmc_clk_update(struct tmio_mmc_host *host)
+static int tmio_mmc_clk_enable(struct tmio_mmc_host *host)
 {
-	struct mmc_host *mmc = host->mmc;
-	int ret;
-
 	if (!host->clk_enable)
 		return -ENOTSUPP;
 
-	ret = host->clk_enable(host);
-	if (!ret)
-		mmc->f_min = mmc->f_max / 512;
-
-	return ret;
+	return host->clk_enable(host);
 }
 
 static void tmio_mmc_power_on(struct tmio_mmc_host *host, unsigned short vdd)
@@ -1135,7 +1131,7 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host,
 				  mmc->caps & MMC_CAP_NONREMOVABLE ||
 				  mmc->slot.cd_irq >= 0);
 
-	if (tmio_mmc_clk_update(_host) < 0) {
+	if (tmio_mmc_clk_enable(_host) < 0) {
 		mmc->f_max = pdata->hclk;
 		mmc->f_min = mmc->f_max / 512;
 	}
@@ -1263,7 +1259,7 @@ int tmio_mmc_host_runtime_resume(struct device *dev)
 	struct tmio_mmc_host *host = mmc_priv(mmc);
 
 	tmio_mmc_reset(host);
-	tmio_mmc_clk_update(host);
+	tmio_mmc_clk_enable(host);
 
 	if (host->clk_cache) {
 		tmio_mmc_set_clock(host, host->clk_cache);
-- 
2.7.0

[PATCH v2 3/9] mmc: tmio: Add UHS-I mode support

[Wolfram Sang]

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

Based on work by Shinobu Uehara and Ben Dooks. This adds the voltage
switch operation needed for all UHS-I modes, but not the tuning needed
for SDR-104 which will come later.

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/tmio_mmc.h     |  2 ++
 drivers/mmc/host/tmio_mmc_pio.c | 12 +++++++++++-
 include/linux/mmc/tmio.h        |  2 ++
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index b44b5890290622..b1819c74965b47 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -101,6 +101,8 @@ struct tmio_mmc_host {
 	void (*clk_disable)(struct tmio_mmc_host *host);
 	int (*multi_io_quirk)(struct mmc_card *card,
 			      unsigned int direction, int blk_size);
+	int (*start_signal_voltage_switch)(struct mmc_host *mmc,
+					   struct mmc_ios *ios);
 };
 
 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 ae81b34f17a5a5..53e5ba5a21914c 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -1012,12 +1012,20 @@ static int tmio_multi_io_quirk(struct mmc_card *card,
 	return blk_size;
 }
 
-static const struct mmc_host_ops tmio_mmc_ops = {
+static int tmio_mmc_card_busy(struct mmc_host *mmc)
+{
+	struct tmio_mmc_host *host = mmc_priv(mmc);
+
+	return !(sd_ctrl_read32(host, CTL_STATUS2) & TMIO_STATUS2_DAT0);
+}
+
+static struct mmc_host_ops tmio_mmc_ops = {
 	.request	= tmio_mmc_request,
 	.set_ios	= tmio_mmc_set_ios,
 	.get_ro         = tmio_mmc_get_ro,
 	.get_cd		= mmc_gpio_get_cd,
 	.enable_sdio_irq = tmio_mmc_enable_sdio_irq,
+	.card_busy	= tmio_mmc_card_busy,
 	.multi_io_quirk	= tmio_multi_io_quirk,
 };
 
@@ -1116,7 +1124,9 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host,
 		goto host_free;
 	}
 
+	tmio_mmc_ops.start_signal_voltage_switch = _host->start_signal_voltage_switch;
 	mmc->ops = &tmio_mmc_ops;
+
 	mmc->caps |= MMC_CAP_4_BIT_DATA | pdata->capabilities;
 	mmc->caps2 |= pdata->capabilities2;
 	mmc->max_segs = 32;
diff --git a/include/linux/mmc/tmio.h b/include/linux/mmc/tmio.h
index 5f5cd80e976500..b2f28e99503383 100644
--- a/include/linux/mmc/tmio.h
+++ b/include/linux/mmc/tmio.h
@@ -63,6 +63,8 @@
 #define TMIO_STAT_CMD_BUSY      0x40000000
 #define TMIO_STAT_ILL_ACCESS    0x80000000
 
+#define TMIO_STATUS2_DAT0	BIT(7)
+
 #define	CLK_CTL_DIV_MASK	0xff
 #define	CLK_CTL_SCLKEN		BIT(8)
 
-- 
2.7.0

[PATCH v2 4/9] mmc: tmio: always start clock after frequency calculation

[Wolfram Sang]

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

Starting the clock is always done after frequency change anyhow, so we can do
it directly after the clock calculation and remove the specific calls. This is
the first part of doing proper clock de-/activation at calculation time.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/tmio_mmc_pio.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index 53e5ba5a21914c..a05938da049bd6 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -154,6 +154,18 @@ static void tmio_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
 	}
 }
 
+static void tmio_mmc_clk_start(struct tmio_mmc_host *host)
+{
+	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, CLK_CTL_SCLKEN |
+		sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
+	msleep(host->pdata->flags & TMIO_MMC_FAST_CLK_CHG ? 1 : 10);
+
+	if (host->pdata->flags & TMIO_MMC_HAVE_HIGH_REG) {
+		sd_ctrl_write16(host, CTL_CLK_AND_WAIT_CTL, 0x0100);
+		msleep(10);
+	}
+}
+
 static void tmio_mmc_set_clock(struct tmio_mmc_host *host,
 				unsigned int new_clock)
 {
@@ -182,6 +194,8 @@ static void tmio_mmc_set_clock(struct tmio_mmc_host *host,
 	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, clk & CLK_CTL_DIV_MASK);
 	if (!(host->pdata->flags & TMIO_MMC_FAST_CLK_CHG))
 		msleep(10);
+
+	tmio_mmc_clk_start(host);
 }
 
 static void tmio_mmc_clk_stop(struct tmio_mmc_host *host)
@@ -196,18 +210,6 @@ static void tmio_mmc_clk_stop(struct tmio_mmc_host *host)
 	msleep(host->pdata->flags & TMIO_MMC_FAST_CLK_CHG ? 5 : 10);
 }
 
-static void tmio_mmc_clk_start(struct tmio_mmc_host *host)
-{
-	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, CLK_CTL_SCLKEN |
-		sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
-	msleep(host->pdata->flags & TMIO_MMC_FAST_CLK_CHG ? 1 : 10);
-
-	if (host->pdata->flags & TMIO_MMC_HAVE_HIGH_REG) {
-		sd_ctrl_write16(host, CTL_CLK_AND_WAIT_CTL, 0x0100);
-		msleep(10);
-	}
-}
-
 static void tmio_mmc_reset(struct tmio_mmc_host *host)
 {
 	/* FIXME - should we set stop clock reg here */
@@ -955,14 +957,12 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 		tmio_mmc_clk_stop(host);
 		break;
 	case MMC_POWER_UP:
-		tmio_mmc_set_clock(host, ios->clock);
 		tmio_mmc_power_on(host, ios->vdd);
-		tmio_mmc_clk_start(host);
+		tmio_mmc_set_clock(host, ios->clock);
 		tmio_mmc_set_bus_width(host, ios->bus_width);
 		break;
 	case MMC_POWER_ON:
 		tmio_mmc_set_clock(host, ios->clock);
-		tmio_mmc_clk_start(host);
 		tmio_mmc_set_bus_width(host, ios->bus_width);
 		break;
 	}
@@ -1271,10 +1271,8 @@ int tmio_mmc_host_runtime_resume(struct device *dev)
 	tmio_mmc_reset(host);
 	tmio_mmc_clk_enable(host);
 
-	if (host->clk_cache) {
+	if (host->clk_cache)
 		tmio_mmc_set_clock(host, host->clk_cache);
-		tmio_mmc_clk_start(host);
-	}
 
 	tmio_mmc_enable_dma(host, true);
 
-- 
2.7.0

[PATCH v2 5/9] mmc: tmio: stop clock when 0Hz is requested

[Wolfram Sang]

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

Setting frequency to 0 is not enough, the clock explicitly has to be
disabled. Otherwise voltage switching (which needs SDCLK to be quiet)
fails for various cards.

Because we now do the 'new_clock == 0' check right at the beginning,
the indentation level of the rest of the code can be decreased a little.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/tmio_mmc_pio.c | 50 +++++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index a05938da049bd6..8e2fef7c5e481c 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -166,25 +166,39 @@ static void tmio_mmc_clk_start(struct tmio_mmc_host *host)
 	}
 }
 
+static void tmio_mmc_clk_stop(struct tmio_mmc_host *host)
+{
+	if (host->pdata->flags & TMIO_MMC_HAVE_HIGH_REG) {
+		sd_ctrl_write16(host, CTL_CLK_AND_WAIT_CTL, 0x0000);
+		msleep(10);
+	}
+
+	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, ~CLK_CTL_SCLKEN &
+		sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
+	msleep(host->pdata->flags & TMIO_MMC_FAST_CLK_CHG ? 5 : 10);
+}
+
 static void tmio_mmc_set_clock(struct tmio_mmc_host *host,
 				unsigned int new_clock)
 {
 	u32 clk = 0, clock;
 
-	if (new_clock) {
-		if (host->clk_update)
-			clock = host->clk_update(host, new_clock) / 512;
-		else
-			clock = host->mmc->f_min;
+	if (new_clock == 0) {
+		tmio_mmc_clk_stop(host);
+		return;
+	}
 
-		for (clk = 0x80000080; new_clock >= (clock << 1); clk >>= 1)
-			clock <<= 1;
+	if (host->clk_update)
+		clock = host->clk_update(host, new_clock) / 512;
+	else
+		clock = host->mmc->f_min;
 
-		/* 1/1 clock is option */
-		if ((host->pdata->flags & TMIO_MMC_CLK_ACTUAL) &&
-		   ((clk >> 22) & 0x1))
-			clk |= 0xff;
-	}
+	for (clk = 0x80000080; new_clock >= (clock << 1); clk >>= 1)
+		clock <<= 1;
+
+	/* 1/1 clock is option */
+	if ((host->pdata->flags & TMIO_MMC_CLK_ACTUAL) && ((clk >> 22) & 0x1))
+		clk |= 0xff;
 
 	if (host->set_clk_div)
 		host->set_clk_div(host->pdev, (clk >> 22) & 1);
@@ -198,18 +212,6 @@ static void tmio_mmc_set_clock(struct tmio_mmc_host *host,
 	tmio_mmc_clk_start(host);
 }
 
-static void tmio_mmc_clk_stop(struct tmio_mmc_host *host)
-{
-	if (host->pdata->flags & TMIO_MMC_HAVE_HIGH_REG) {
-		sd_ctrl_write16(host, CTL_CLK_AND_WAIT_CTL, 0x0000);
-		msleep(10);
-	}
-
-	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, ~CLK_CTL_SCLKEN &
-		sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
-	msleep(host->pdata->flags & TMIO_MMC_FAST_CLK_CHG ? 5 : 10);
-}
-
 static void tmio_mmc_reset(struct tmio_mmc_host *host)
 {
 	/* FIXME - should we set stop clock reg here */
-- 
2.7.0

[PATCH v2 6/9] mmc: host: add note that set_ios needs to handle 0Hz properly

[Wolfram Sang]

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

While here, refactor the comments so that they are before the
declaration they are referring to.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 include/linux/mmc/host.h | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 8dd4d290ab0d86..85800b48241fad 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -93,28 +93,39 @@ struct mmc_host_ops {
 	void	(*pre_req)(struct mmc_host *host, struct mmc_request *req,
 			   bool is_first_req);
 	void	(*request)(struct mmc_host *host, struct mmc_request *req);
+
+	/*
+	 * Avoid calling the next three functions too often or in a "fast
+	 * path", since underlaying controller might implement them in an
+	 * expensive and/or slow way. Also note that these functions might
+	 * sleep, so don't call them in the atomic contexts!
+	 */
+
+	/*
+	 * Notes to the set_ios callback:
+	 * ios->clock might be 0. For some controllers, setting 0Hz
+	 * as any other frequency works. However, some controllers
+	 * explicitly need to disable the clock. Otherwise e.g. voltage
+	 * switching might fail because the SDCLK is not really quiet.
+	 */
+	void	(*set_ios)(struct mmc_host *host, struct mmc_ios *ios);
+
 	/*
-	 * Avoid calling these three functions too often or in a "fast path",
-	 * since underlaying controller might implement them in an expensive
-	 * and/or slow way.
-	 *
-	 * Also note that these functions might sleep, so don't call them
-	 * in the atomic contexts!
-	 *
 	 * Return values for the get_ro callback should be:
 	 *   0 for a read/write card
 	 *   1 for a read-only card
 	 *   -ENOSYS when not supported (equal to NULL callback)
 	 *   or a negative errno value when something bad happened
-	 *
+	 */
+	int	(*get_ro)(struct mmc_host *host);
+
+	/*
 	 * Return values for the get_cd callback should be:
 	 *   0 for a absent card
 	 *   1 for a present card
 	 *   -ENOSYS when not supported (equal to NULL callback)
 	 *   or a negative errno value when something bad happened
 	 */
-	void	(*set_ios)(struct mmc_host *host, struct mmc_ios *ios);
-	int	(*get_ro)(struct mmc_host *host);
 	int	(*get_cd)(struct mmc_host *host);
 
 	void	(*enable_sdio_irq)(struct mmc_host *host, int enable);
-- 
2.7.0

[PATCH v2 7/9] mmc: sh_mobile_sdhi: Add UHS-I mode support

[Wolfram Sang]

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

Implement voltage switch, supporting modes up to SDR-50.

Based on work by Shinobu Uehara, Rob Taylor, William Towle and Ian Molton.

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 Documentation/devicetree/bindings/mmc/tmio_mmc.txt |  3 ++
 drivers/mmc/host/sh_mobile_sdhi.c                  | 50 ++++++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
index 7fb746dd1a68ca..0f610d4b5b005f 100644
--- a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
@@ -26,3 +26,6 @@ Required properties:
 
 Optional properties:
 - toshiba,mmc-wrprotect-disable: write-protect detection is unavailable
+- pinctrl-names: should be "default", "state_uhs"
+- pinctrl-0: should contain default/high speed pin ctrl
+- pinctrl-1: should contain uhs mode pin ctrl
diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 8fd1d6b29190b6..89e0c5e9fcc668 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -32,6 +32,9 @@
 #include <linux/mfd/tmio.h>
 #include <linux/sh_dma.h>
 #include <linux/delay.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/pinctrl/pinctrl-state.h>
+#include <linux/regulator/consumer.h>
 
 #include "tmio_mmc.h"
 
@@ -97,6 +100,8 @@ struct sh_mobile_sdhi {
 	struct clk *clk;
 	struct tmio_mmc_data mmc_data;
 	struct tmio_mmc_dma dma_priv;
+	struct pinctrl *pinctrl;
+	struct pinctrl_state *pins_default, *pins_uhs;
 };
 
 static void sh_mobile_sdhi_sdbuf_width(struct tmio_mmc_host *host, int width)
@@ -205,6 +210,44 @@ static void sh_mobile_sdhi_clk_disable(struct tmio_mmc_host *host)
 	clk_disable_unprepare(priv->clk);
 }
 
+static int sh_mobile_sdhi_start_signal_voltage_switch(struct mmc_host *mmc,
+						      struct mmc_ios *ios)
+{
+	struct tmio_mmc_host *host = mmc_priv(mmc);
+	struct sh_mobile_sdhi *priv = host_to_priv(host);
+	struct pinctrl_state *pin_state;
+	int ret;
+
+	switch (ios->signal_voltage) {
+	case MMC_SIGNAL_VOLTAGE_330:
+		pin_state = priv->pins_default;
+		break;
+	case MMC_SIGNAL_VOLTAGE_180:
+		pin_state = priv->pins_uhs;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/*
+	 * If anything is missing, assume signal voltage is fixed at
+	 * 3.3V and succeed/fail accordingly.
+	 */
+	if (IS_ERR(priv->pinctrl) || IS_ERR(pin_state))
+		return ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330 ? 0 : -EINVAL;
+
+	ret = mmc_regulator_set_vqmmc(host->mmc, ios);
+	if (ret)
+		return ret;
+
+	ret = pinctrl_select_state(priv->pinctrl, pin_state);
+	if (ret)
+		return ret;
+
+	usleep_range(5000, 5500);
+	return 0;
+}
+
 static int sh_mobile_sdhi_wait_idle(struct tmio_mmc_host *host)
 {
 	int timeout = 1000;
@@ -296,6 +339,12 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 		goto eprobe;
 	}
 
+	priv->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (!IS_ERR(priv->pinctrl)) {
+		priv->pins_default = pinctrl_lookup_state(priv->pinctrl, PINCTRL_STATE_DEFAULT);
+		priv->pins_uhs = pinctrl_lookup_state(priv->pinctrl, "state_uhs");
+	}
+
 	host = tmio_mmc_host_alloc(pdev);
 	if (!host) {
 		ret = -ENOMEM;
@@ -319,6 +368,7 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 	host->clk_update	= sh_mobile_sdhi_clk_update;
 	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;
 
 	/* 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 */
-- 
2.7.0

[PATCH v2 8/9] ARM: shmobile: r8a7790: Set maximum frequencies for SDHI clocks

[Wolfram Sang]

From: Ben Hutchings <ben.hutchings@codethink.co.uk>

Taken from the datasheet.

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 arch/arm/boot/dts/r8a7790.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi
index abd7d88c411e4a..1a3b6b08f1f3c9 100644
--- a/arch/arm/boot/dts/r8a7790.dtsi
+++ b/arch/arm/boot/dts/r8a7790.dtsi
@@ -607,6 +607,7 @@
 		dmas = <&dmac1 0xcd>, <&dmac1 0xce>,
 		       <&dmac0 0xcd>, <&dmac0 0xce>;
 		dma-names = "tx", "rx", "tx", "rx";
+		max-frequency = <156000000>;
 		power-domains = <&cpg_clocks>;
 		status = "disabled";
 	};
@@ -619,6 +620,7 @@
 		dmas = <&dmac1 0xc9>, <&dmac1 0xca>,
 		       <&dmac0 0xc9>, <&dmac0 0xca>;
 		dma-names = "tx", "rx", "tx", "rx";
+		max-frequency = <156000000>;
 		power-domains = <&cpg_clocks>;
 		status = "disabled";
 	};
@@ -631,6 +633,7 @@
 		dmas = <&dmac1 0xc1>, <&dmac1 0xc2>,
 		       <&dmac0 0xc1>, <&dmac0 0xc2>;
 		dma-names = "tx", "rx", "tx", "rx";
+		max-frequency = <97500000>;
 		power-domains = <&cpg_clocks>;
 		status = "disabled";
 	};
@@ -643,6 +646,7 @@
 		dmas = <&dmac1 0xd3>, <&dmac1 0xd4>,
 		       <&dmac0 0xd3>, <&dmac0 0xd4>;
 		dma-names = "tx", "rx", "tx", "rx";
+		max-frequency = <97500000>;
 		power-domains = <&cpg_clocks>;
 		status = "disabled";
 	};
-- 
2.7.0

[PATCH v2 9/9] ARM: shmobile: r8a7790: lager: Enable UHS-I SDR-50

[Wolfram Sang]

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

Add the "1v8" pinctrl state and sd-uhs-sdr50 property to SDHI{0,2}.

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 arch/arm/boot/dts/r8a7790-lager.dts | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
index 454fce0c5b8d12..4b8d9fc94c9658 100644
--- a/arch/arm/boot/dts/r8a7790-lager.dts
+++ b/arch/arm/boot/dts/r8a7790-lager.dts
@@ -345,11 +345,25 @@
 	sdhi0_pins: sd0 {
 		groups = "sdhi0_data4", "sdhi0_ctrl";
 		function = "sdhi0";
+		power-source = <3300>;
+	};
+
+	sdhi0_pins_uhs: sd0_uhs {
+		groups = "sdhi0_data4", "sdhi0_ctrl";
+		function = "sdhi0";
+		power-source = <1800>;
 	};
 
 	sdhi2_pins: sd2 {
 		groups = "sdhi2_data4", "sdhi2_ctrl";
 		function = "sdhi2";
+		power-source = <3300>;
+	};
+
+	sdhi2_pins_uhs: sd2_uhs {
+		groups = "sdhi2_data4", "sdhi2_ctrl";
+		function = "sdhi2";
+		power-source = <1800>;
 	};
 
 	mmc1_pins: mmc1 {
@@ -538,21 +552,25 @@
 
 &sdhi0 {
 	pinctrl-0 = <&sdhi0_pins>;
-	pinctrl-names = "default";
+	pinctrl-1 = <&sdhi0_pins_uhs>;
+	pinctrl-names = "default", "state_uhs";
 
 	vmmc-supply = <&vcc_sdhi0>;
 	vqmmc-supply = <&vccq_sdhi0>;
 	cd-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>;
+	sd-uhs-sdr50;
 	status = "okay";
 };
 
 &sdhi2 {
 	pinctrl-0 = <&sdhi2_pins>;
-	pinctrl-names = "default";
+	pinctrl-1 = <&sdhi2_pins_uhs>;
+	pinctrl-names = "default", "state_uhs";
 
 	vmmc-supply = <&vcc_sdhi2>;
 	vqmmc-supply = <&vccq_sdhi2>;
 	cd-gpios = <&gpio3 22 GPIO_ACTIVE_LOW>;
+	sd-uhs-sdr50;
 	status = "okay";
 };
 
-- 
2.7.0

Re: [PATCH v2 7/9] mmc: sh_mobile_sdhi: Add UHS-I mode support

[Ulf Hansson]

On 1 April 2016 at 17:44, Wolfram Sang <wsa@the-dreams.de> wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> Implement voltage switch, supporting modes up to SDR-50.
>
> Based on work by Shinobu Uehara, Rob Taylor, William Towle and Ian Molton.
>
> Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  Documentation/devicetree/bindings/mmc/tmio_mmc.txt |  3 ++
>  drivers/mmc/host/sh_mobile_sdhi.c                  | 50 ++++++++++++++++++++++
>  2 files changed, 53 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
> index 7fb746dd1a68ca..0f610d4b5b005f 100644
> --- a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
> @@ -26,3 +26,6 @@ Required properties:
>
>  Optional properties:
>  - toshiba,mmc-wrprotect-disable: write-protect detection is unavailable
> +- pinctrl-names: should be "default", "state_uhs"
> +- pinctrl-0: should contain default/high speed pin ctrl
> +- pinctrl-1: should contain uhs mode pin ctrl
> diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
> index 8fd1d6b29190b6..89e0c5e9fcc668 100644
> --- a/drivers/mmc/host/sh_mobile_sdhi.c
> +++ b/drivers/mmc/host/sh_mobile_sdhi.c
> @@ -32,6 +32,9 @@
>  #include <linux/mfd/tmio.h>
>  #include <linux/sh_dma.h>
>  #include <linux/delay.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/pinctrl/pinctrl-state.h>
> +#include <linux/regulator/consumer.h>
>
>  #include "tmio_mmc.h"
>
> @@ -97,6 +100,8 @@ struct sh_mobile_sdhi {
>         struct clk *clk;
>         struct tmio_mmc_data mmc_data;
>         struct tmio_mmc_dma dma_priv;
> +       struct pinctrl *pinctrl;
> +       struct pinctrl_state *pins_default, *pins_uhs;
>  };
>
>  static void sh_mobile_sdhi_sdbuf_width(struct tmio_mmc_host *host, int width)
> @@ -205,6 +210,44 @@ static void sh_mobile_sdhi_clk_disable(struct tmio_mmc_host *host)
>         clk_disable_unprepare(priv->clk);
>  }
>
> +static int sh_mobile_sdhi_start_signal_voltage_switch(struct mmc_host *mmc,
> +                                                     struct mmc_ios *ios)
> +{
> +       struct tmio_mmc_host *host = mmc_priv(mmc);
> +       struct sh_mobile_sdhi *priv = host_to_priv(host);
> +       struct pinctrl_state *pin_state;
> +       int ret;
> +
> +       switch (ios->signal_voltage) {
> +       case MMC_SIGNAL_VOLTAGE_330:
> +               pin_state = priv->pins_default;
> +               break;
> +       case MMC_SIGNAL_VOLTAGE_180:
> +               pin_state = priv->pins_uhs;
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       /*
> +        * If anything is missing, assume signal voltage is fixed at
> +        * 3.3V and succeed/fail accordingly.
> +        */
> +       if (IS_ERR(priv->pinctrl) || IS_ERR(pin_state))
> +               return ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330 ? 0 : -EINVAL;
> +
> +       ret = mmc_regulator_set_vqmmc(host->mmc, ios);
> +       if (ret)
> +               return ret;
> +
> +       ret = pinctrl_select_state(priv->pinctrl, pin_state);
> +       if (ret)
> +               return ret;
> +
> +       usleep_range(5000, 5500);

Why do you need this delay/sleep?

> +       return 0;
> +}
> +
>  static int sh_mobile_sdhi_wait_idle(struct tmio_mmc_host *host)
>  {
>         int timeout = 1000;
> @@ -296,6 +339,12 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
>                 goto eprobe;
>         }
>
> +       priv->pinctrl = devm_pinctrl_get(&pdev->dev);
> +       if (!IS_ERR(priv->pinctrl)) {
> +               priv->pins_default = pinctrl_lookup_state(priv->pinctrl, PINCTRL_STATE_DEFAULT);
> +               priv->pins_uhs = pinctrl_lookup_state(priv->pinctrl, "state_uhs");
> +       }
> +
>         host = tmio_mmc_host_alloc(pdev);
>         if (!host) {
>                 ret = -ENOMEM;
> @@ -319,6 +368,7 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
>         host->clk_update        = sh_mobile_sdhi_clk_update;
>         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;
>
>         /* 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 */
> --
> 2.7.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Kind regards
Uffe

Re: [PATCH v2 0/9] r8a7790: add UHS-I (SDR50) support to Lager

[Ulf Hansson]

On 1 April 2016 at 17:44, Wolfram Sang <wsa@the-dreams.de> wrote:
> So, here is v2 of the series adding SDR50 support to the Renesas Lager board.
> Change since v1:
>
> * rebased to renesas-drivers based on v4.6-rc1
> * dropped pinctrl patch which was already picked up
> * directly populate start_signal_voltage_switch() in mmc_ops
>   instead of using a wrapper function
> * use generic DT pinctrl bindings instead of vendor specific ones
>   (e.g. "groups" instead of "renesas,groups")
>
> The RFC series worked fine with my Transcend card, but failed to switch
> voltages on a SanDisk and Samsung card. This bug hunting resulted in patches
> 4-6 newly added: The clock really has to be disabled on ios->clock == 0,
> setting frequency to 0Hz doesn't work. I wonder if this isn't true for some
> more controllers? Is this known already?

The documentation of mmc is quite poor. There are both code and
specification that needs to be understand, so some more help from
kernel doc could clearly help.

Anyway, I noticed that patch 6 helps to improve some documentation,
that's great - thanks!

>
> My "copy-large-files-around"-setup showed now 30MB/s while it had 19MB/s
> without SDR50. Frankly, I hoped for a little more, but let's start with this
> initial support and do the tuning incrementally I'd say.
>
> Patches 1-7 should go via Ulf. After those patches went in, Simon can take 8+9
> to tie it all together.
>
> Please comment, test, apply...

I only had one minor comment on patch 7, the rest looks good to me!

[...]

Kind regards
Uffe

Re: [PATCH v2 7/9] mmc: sh_mobile_sdhi: Add UHS-I mode support

[Wolfram Sang]

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


> > +       usleep_range(5000, 5500);
> 
> Why do you need this delay/sleep?

My datasheet says to wait at least 5ms before re-enabling the clock
after a voltage switch...

Ah, found it in the core. It is doing it for 10ms. Maybe my search
pattern was not fuzzy enough ;)

Okay, so this delay can go.


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

Re: [PATCH v2 0/9] r8a7790: add UHS-I (SDR50) support to Lager

[Wolfram Sang]

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


> The documentation of mmc is quite poor. There are both code and
> specification that needs to be understand, so some more help from
> kernel doc could clearly help.

I agree.

> Anyway, I noticed that patch 6 helps to improve some documentation,
> that's great - thanks!

You're welcome. It wasn't too much work and maybe it helps someone with
debugging. I found some bug reports on the web which sounded somewhat
similar.

> I only had one minor comment on patch 7, the rest looks good to me!

Do you want me to resend or could you fix it before applying?

Thanks for the review!

   Wolfram


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

Re: [PATCH v2 7/9] mmc: sh_mobile_sdhi: Add UHS-I mode support

[Ulf Hansson]

On 4 April 2016 at 17:17, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> > +       usleep_range(5000, 5500);
>>
>> Why do you need this delay/sleep?
>
> My datasheet says to wait at least 5ms before re-enabling the clock
> after a voltage switch...
>
> Ah, found it in the core. It is doing it for 10ms. Maybe my search
> pattern was not fuzzy enough ;)
>
> Okay, so this delay can go.
>

Thanks for the clarification. I will amend the patch, no need for you
to send a new version!

Kind regards
Uffe

Re: [PATCH v2 7/9] mmc: sh_mobile_sdhi: Add UHS-I mode support

[Wolfram Sang]

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


> > Okay, so this delay can go.
> >
> 
> Thanks for the clarification. I will amend the patch, no need for you
> to send a new version!

Thanks a lot!


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

Re: [PATCH v2 0/9] r8a7790: add UHS-I (SDR50) support to Lager

[Ulf Hansson]

On 4 April 2016 at 17:21, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> The documentation of mmc is quite poor. There are both code and
>> specification that needs to be understand, so some more help from
>> kernel doc could clearly help.
>
> I agree.
>
>> Anyway, I noticed that patch 6 helps to improve some documentation,
>> that's great - thanks!
>
> You're welcome. It wasn't too much work and maybe it helps someone with
> debugging. I found some bug reports on the web which sounded somewhat
> similar.
>
>> I only had one minor comment on patch 7, the rest looks good to me!
>
> Do you want me to resend or could you fix it before applying?

Patch 1->7 is applied for next (amended patch7), thanks!

Kind regards
Uffe

Re: [PATCH v2 0/9] r8a7790: add UHS-I (SDR50) support to Lager

[Wolfram Sang]

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


> > Do you want me to resend or could you fix it before applying?
> 
> Patch 1->7 is applied for next (amended patch7), thanks!

Thanks! Simon, can you pick up patches 8+9 now?

Next series coming soon... ;)


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

Re: [PATCH v2 0/9] r8a7790: add UHS-I (SDR50) support to Lager

[Simon Horman]

On Tue, Apr 05, 2016 at 01:19:20PM +0200, Wolfram Sang wrote:
> 
> > > Do you want me to resend or could you fix it before applying?
> > 
> > Patch 1->7 is applied for next (amended patch7), thanks!
> 
> Thanks! Simon, can you pick up patches 8+9 now?

Thanks, done!

Re: [v2,9/9] ARM: shmobile: r8a7790: lager: Enable UHS-I SDR-50

[Simon Horman]

On Fri, Apr 01, 2016 at 05:44:39PM +0200, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Add the "1v8" pinctrl state and sd-uhs-sdr50 property to SDHI{0,2}.

For some reason which I am entirely unsure of git didn't apply this cleanly
so I did so manually. Please double check that the following is correct.

From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Subject: [PATCH] ARM: shmobile: r8a7790: lager: Enable UHS-I SDR-50

Add the "1v8" pinctrl state and sd-uhs-sdr50 property to SDHI{0,2}.

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
 arch/arm/boot/dts/r8a7790-lager.dts | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
index aa6ca92a9485..75fc8d1cb7e3 100644
--- a/arch/arm/boot/dts/r8a7790-lager.dts
+++ b/arch/arm/boot/dts/r8a7790-lager.dts
@@ -346,11 +346,25 @@
 	sdhi0_pins: sd0 {
 		renesas,groups = "sdhi0_data4", "sdhi0_ctrl";
 		renesas,function = "sdhi0";
+		power-source = <3300>;
+	};
+
+	sdhi0_pins_uhs: sd0_uhs {
+		groups = "sdhi0_data4", "sdhi0_ctrl";
+		function = "sdhi0";
+		power-source = <1800>;
 	};
 
 	sdhi2_pins: sd2 {
 		renesas,groups = "sdhi2_data4", "sdhi2_ctrl";
 		renesas,function = "sdhi2";
+		power-source = <3300>;
+	};
+
+	sdhi2_pins_uhs: sd2_uhs {
+		groups = "sdhi2_data4", "sdhi2_ctrl";
+		function = "sdhi2";
+		power-source = <1800>;
 	};
 
 	mmc1_pins: mmc1 {
@@ -539,21 +553,25 @@
 
 &sdhi0 {
 	pinctrl-0 = <&sdhi0_pins>;
-	pinctrl-names = "default";
+	pinctrl-1 = <&sdhi0_pins_uhs>;
+	pinctrl-names = "default", "state_uhs";
 
 	vmmc-supply = <&vcc_sdhi0>;
 	vqmmc-supply = <&vccq_sdhi0>;
 	cd-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>;
+	sd-uhs-sdr50;
 	status = "okay";
 };
 
 &sdhi2 {
 	pinctrl-0 = <&sdhi2_pins>;
-	pinctrl-names = "default";
+	pinctrl-1 = <&sdhi2_pins_uhs>;
+	pinctrl-names = "default", "state_uhs";
 
 	vmmc-supply = <&vcc_sdhi2>;
 	vqmmc-supply = <&vccq_sdhi2>;
 	cd-gpios = <&gpio3 22 GPIO_ACTIVE_LOW>;
+	sd-uhs-sdr50;
 	status = "okay";
 };
 
-- 
2.1.4

Re: [v2,9/9] ARM: shmobile: r8a7790: lager: Enable UHS-I SDR-50

[Geert Uytterhoeven]

Hi Simon,

On Wed, Apr 6, 2016 at 2:56 AM, Simon Horman <horms@verge.net.au> wrote:
> On Fri, Apr 01, 2016 at 05:44:39PM +0200, Wolfram Sang wrote:
>> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
>>
>> Add the "1v8" pinctrl state and sd-uhs-sdr50 property to SDHI{0,2}.
>
> For some reason which I am entirely unsure of git didn't apply this cleanly
> so I did so manually. Please double check that the following is correct.
>
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Subject: [PATCH] ARM: shmobile: r8a7790: lager: Enable UHS-I SDR-50
>
> Add the "1v8" pinctrl state and sd-uhs-sdr50 property to SDHI{0,2}.
>
> Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> ---
>  arch/arm/boot/dts/r8a7790-lager.dts | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
> index aa6ca92a9485..75fc8d1cb7e3 100644
> --- a/arch/arm/boot/dts/r8a7790-lager.dts
> +++ b/arch/arm/boot/dts/r8a7790-lager.dts
> @@ -346,11 +346,25 @@
>         sdhi0_pins: sd0 {
>                 renesas,groups = "sdhi0_data4", "sdhi0_ctrl";
>                 renesas,function = "sdhi0";

How come you still have "renesas,*" pinctrl properties?
Applied to wrong branch?

> +               power-source = <3300>;
> +       };
> +
> +       sdhi0_pins_uhs: sd0_uhs {
> +               groups = "sdhi0_data4", "sdhi0_ctrl";
> +               function = "sdhi0";

Pinctrl properties with and without prefixes must not be combined in a single
preprocessed DTS (and in the overlays).

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: [v2,9/9] ARM: shmobile: r8a7790: lager: Enable UHS-I SDR-50

[Simon Horman]

On Wed, Apr 06, 2016 at 08:38:02AM +0200, Geert Uytterhoeven wrote:
> Hi Simon,
> 
> On Wed, Apr 6, 2016 at 2:56 AM, Simon Horman <horms@verge.net.au> wrote:
> > On Fri, Apr 01, 2016 at 05:44:39PM +0200, Wolfram Sang wrote:
> >> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> >>
> >> Add the "1v8" pinctrl state and sd-uhs-sdr50 property to SDHI{0,2}.
> >
> > For some reason which I am entirely unsure of git didn't apply this cleanly
> > so I did so manually. Please double check that the following is correct.
> >
> > From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > Subject: [PATCH] ARM: shmobile: r8a7790: lager: Enable UHS-I SDR-50
> >
> > Add the "1v8" pinctrl state and sd-uhs-sdr50 property to SDHI{0,2}.
> >
> > Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> > ---
> >  arch/arm/boot/dts/r8a7790-lager.dts | 22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
> > index aa6ca92a9485..75fc8d1cb7e3 100644
> > --- a/arch/arm/boot/dts/r8a7790-lager.dts
> > +++ b/arch/arm/boot/dts/r8a7790-lager.dts
> > @@ -346,11 +346,25 @@
> >         sdhi0_pins: sd0 {
> >                 renesas,groups = "sdhi0_data4", "sdhi0_ctrl";
> >                 renesas,function = "sdhi0";
> 
> How come you still have "renesas,*" pinctrl properties?
> Applied to wrong branch?

Yes, its  branch thing. This is applied to the dt-for-v4.7 branch.
The cleanup is in the cleanup-for-v4.7 branch. I didn't realise that
until after my previous post.

> > +               power-source = <3300>;
> > +       };
> > +
> > +       sdhi0_pins_uhs: sd0_uhs {
> > +               groups = "sdhi0_data4", "sdhi0_ctrl";
> > +               function = "sdhi0";
> 
> Pinctrl properties with and without prefixes must not be combined in a single
> preprocessed DTS (and in the overlays).

Thanks! I will re-arrange by branches accordingly.

Re: [PATCH v2 2/9] mmc: tmio, sh_mobile_sdhi: Add support for variable input clock frequency

[Geert Uytterhoeven]

On Fri, Apr 1, 2016 at 5:44 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> From: Ben Hutchings <ben.hutchings@codethink.co.uk>
>
> Currently tmio_mmc assumes that the input clock frequency is fixed and
> only its own clock divider can be changed.  This is not true in the
> case of sh_mobile_sdhi; we can use the clock API to change it.
>
> In tmio_mmc:
> - Delegate setting of f_min from tmio to the clk_enable operation (if
>   implemented), as it can be smaller than f_max / 512
> - Add an optional clk_update operation called from tmio_mmc_set_clock()
>   that updates the input clock frequency
> - Rename tmio_mmc_clk_update() to tmio_mmc_clk_enable(), to avoid
>   confusion with the clk_update operation
>
> In sh_mobile_sdhi:
> - Make the setting of f_max conditional; it should be set through the
>   max-frequency property in the device tree in future
> - Set f_min based on the input clock's minimum frequency
> - Implement the clk_update operation, selecting the best input clock
>   frequency for the bus frequency that's wanted
>
> sh_mobile_sdhi_clk_update() is loosely based on Kuninori Morimoto's work
> in sh_mmcif.
>
> Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

This is now commit 2e21101df4fe8bdc ("mmc: tmio, sh_mobile_sdhi: Add support
for variable input clock frequency") in the next branch of Ulf's mmc.git.

  1. The SDHI/MMC clocks now run much slower than before. Perhaps this is
     intentional, and a consequence of finding the best way to drive the SD
     card at the target frequency?
  2. On r8a7740, the situation is worse: the HP ("High-speed Peripheral")
     clock is also scaled down from 99 MHz to 12.375 MHz.
     As the HP clock is the parent of lots of on-chip devices, this may affect
     performance for all of them.

On r8a73a4, r8a7791, and sh73a0, the SDHI clocks are children of the pll1_div2
clocks, which are fixed.
On r8a7740, the SDHI and MMC clocks are children of the HP clock,
which is also scaled down, affecting all other siblings.

dmesg and /sys/kernel/debug/clk/clk_summary differences below.


r8a73a4/ape6evm
---------------

-sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 88 MHz
+sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 max clock rate 88 MHz

-sh_mobile_sdhi ee120000.sd: mmc1 base at 0xee120000 clock rate 12 MHz
+sh_mobile_sdhi ee120000.sd: mmc1 base at 0xee120000 max clock rate 12 MHz

   clock           enable_cnt  prepare_cnt        rate   accuracy   phase
-------------------------------------------------------------------------

-     sdhi1ck               1            1    12500000          0 0
-        sdhi1              2            2    12500000          0 0
-     sdhi0ck               1            1    88888888          0 0
-        sdhi0              1            2    88888888          0 0
+     sdhi1ck               1            1    12698412          0 0
+        sdhi1              2            2    12698412          0 0
+     sdhi0ck               1            1    12698412          0 0
+        sdhi0              1            2    12698412          0 0


r8a7791/koelsch
---------------

-sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 97 MHz
+sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 max clock rate 97 MHz

-sh_mobile_sdhi ee140000.sd: mmc1 base at 0xee140000 clock rate 48 MHz
+sh_mobile_sdhi ee140000.sd: mmc1 base at 0xee140000 max clock rate 48 MHz

-sh_mobile_sdhi ee160000.sd: mmc2 base at 0xee160000 clock rate 48 MHz
+sh_mobile_sdhi ee160000.sd: mmc2 base at 0xee160000 max clock rate 48 MHz


      mmc0                  0            0    12187500          0 0
         mmcif0             0            0    12187500          0 0
-     sd3                   1            1    48750000          0 0
-        sdhi2              1            2    48750000          0 0
-     sd2                   1            1    48750000          0 0
-        sdhi1              1            2    48750000          0 0
+     sd3                   1            1    12786885          0 0
+        sdhi2              1            2    12786885          0 0
+     sd2                   1            1    12786885          0 0
+        sdhi1              1            2    12786885          0 0


sh73a0/kzm9g
------------

-sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 69 MHz
+sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 max clock rate 69 MHz

-sh_mobile_sdhi ee140000.sd: mmc1 base at 0xee140000 clock rate 69 MHz
+sh_mobile_sdhi ee140000.sd: mmc1 base at 0xee140000 max clock rate 69 MHz


-    sdhi2ck                1            1    69333333          0 0
-       sdhi2               2            2    69333333          0 0
+    sdhi2ck                1            1    12734693          0 0
+       sdhi2               2            2    12734693          0 0
     sdhi1ck                0            0    69333333          0 0
        sdhi1               0            0    69333333          0 0
-    sdhi0ck                1            1    69333333          0 0
-       sdhi0               1            2    69333333          0 0
+    sdhi0ck                1            1    12734693          0 0
+       sdhi0               1            2    12734693          0 0


r8a7740/armadillo
-----------------

-sh_mobile_sdhi e6850000.sd: mmc0 base at 0xe6850000 clock rate 99 MHz
+sh_mobile_sdhi e6850000.sd: mmc0 base at 0xe6850000 max clock rate 99 MHz

-sh_mmcif e6bd0000.mmc: Chip version 0x0003, clock rate 99MHz
+sh_mmcif e6bd0000.mmc: Chip version 0x0003, clock rate 12MHz

-   hp                      4            6    99000000          0 0
-      tpu0                 0            1    99000000          0 0
-      gether               1            1    99000000          0 0
-      mmc                  2            2    99000000          0 0
-      sdhi1                0            0    99000000          0 0
-      sdhi0                1            2    99000000          0 0
-      usbf                 0            0    99000000          0 0
-      fsi                  0            1    99000000          0 0
-      usbdmac              0            0    99000000          0 0
-      dmac3                0            0    99000000          0 0
-      dmac2                0            0    99000000          0 0
-      dmac1                0            0    99000000          0 0
-      intca                4            4    99000000          0 0
-      usphy                0            0    99000000          0 0
-      usbfunc              0            0    99000000          0 0
-      sdhi2                0            0    99000000          0 0
-      usbhost              0            0    99000000          0 0
+   hp                      4            6    12375000          0 0
+      tpu0                 0            1    12375000          0 0
+      gether               1            1    12375000          0 0
+      mmc                  2            2    12375000          0 0
+      sdhi1                0            0    12375000          0 0
+      sdhi0                1            2    12375000          0 0
+      usbf                 0            0    12375000          0 0
+      fsi                  0            1    12375000          0 0
+      usbdmac              0            0    12375000          0 0
+      dmac3                0            0    12375000          0 0
+      dmac2                0            0    12375000          0 0
+      dmac1                0            0    12375000          0 0
+      intca                4            4    12375000          0 0
+      usphy                0            0    12375000          0 0
+      usbfunc              0            0    12375000          0 0
+      sdhi2                0            0    12375000          0 0
+      usbhost              0            0    12375000          0 0

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 v2 2/9] mmc: tmio, sh_mobile_sdhi: Add support for variable input clock frequency

[Ben Hutchings]

On Tue, 2016-04-12 at 14:55 +0200, Geert Uytterhoeven wrote:
> On Fri, Apr 1, 2016 at 5:44 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> > From: Ben Hutchings <ben.hutchings@codethink.co.uk>
> >
> > Currently tmio_mmc assumes that the input clock frequency is fixed and
> > only its own clock divider can be changed.  This is not true in the
> > case of sh_mobile_sdhi; we can use the clock API to change it.
> >
> > In tmio_mmc:
> > - Delegate setting of f_min from tmio to the clk_enable operation (if
> >   implemented), as it can be smaller than f_max / 512
> > - Add an optional clk_update operation called from tmio_mmc_set_clock()
> >   that updates the input clock frequency
> > - Rename tmio_mmc_clk_update() to tmio_mmc_clk_enable(), to avoid
> >   confusion with the clk_update operation
> >
> > In sh_mobile_sdhi:
> > - Make the setting of f_max conditional; it should be set through the
> >   max-frequency property in the device tree in future
> > - Set f_min based on the input clock's minimum frequency
> > - Implement the clk_update operation, selecting the best input clock
> >   frequency for the bus frequency that's wanted
> >
> > sh_mobile_sdhi_clk_update() is loosely based on Kuninori Morimoto's work
> > in sh_mmcif.
> >
> > Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> This is now commit 2e21101df4fe8bdc ("mmc: tmio, sh_mobile_sdhi: Add support
> for variable input clock frequency") in the next branch of Ulf's mmc.git.
> 
>   1. The SDHI/MMC clocks now run much slower than before. Perhaps this is
>      intentional, and a consequence of finding the best way to drive the SD
>      card at the target frequency?

I don't think is generally a problem.  Probably even saves a little
power.

>   2. On r8a7740, the situation is worse: the HP ("High-speed Peripheral")
>      clock is also scaled down from 99 MHz to 12.375 MHz.
>      As the HP clock is the parent of lots of on-chip devices, this may affect
>      performance for all of them.
>
> On r8a73a4, r8a7791, and sh73a0, the SDHI clocks are children of the pll1_div2
> clocks, which are fixed.
> On r8a7740, the SDHI and MMC clocks are children of the HP clock,
> which is also scaled down, affecting all other siblings.
[...]

That seems like a bug in the clock driver.  If it doesn't have
independent dividers for each clock client then it shouldn't allow any
client to change the frequency.

Ben.

-- 
Ben Hutchings
Software Developer, Codethink Ltd.

Re: [PATCH v2 2/9] mmc: tmio, sh_mobile_sdhi: Add support for variable input clock frequency

[Wolfram Sang]

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


> >   1. The SDHI/MMC clocks now run much slower than before. Perhaps this is
> >      intentional, and a consequence of finding the best way to drive the SD
> >      card at the target frequency?
> 
> I don't think is generally a problem.  Probably even saves a little
> power.

If you insert an SD card, frequencies should be back to normal. This
happens on Lager at least.

> 
> >   2. On r8a7740, the situation is worse: the HP ("High-speed Peripheral")
> >      clock is also scaled down from 99 MHz to 12.375 MHz.
> >      As the HP clock is the parent of lots of on-chip devices, this may affect
> >      performance for all of them.
> >
> > On r8a73a4, r8a7791, and sh73a0, the SDHI clocks are children of the pll1_div2
> > clocks, which are fixed.
> > On r8a7740, the SDHI and MMC clocks are children of the HP clock,
> > which is also scaled down, affecting all other siblings.
> [...]
> 
> That seems like a bug in the clock driver.  If it doesn't have
> independent dividers for each clock client then it shouldn't allow any
> client to change the frequency.

I tend to agree.

However, I just found out that we don't check the result of
clk_set_rate(). Probably something like this is missing?


diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 5923ce7e0fccb3..e51d7b01d39a3b 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -167,7 +167,7 @@ static unsigned int sh_mobile_sdhi_clk_update(struct tmio_mmc_host *host,
 {
 	struct sh_mobile_sdhi *priv = host_to_priv(host);
 	unsigned int freq, best_freq, diff_min, diff;
-	int i;
+	int i, ret;
 
 	diff_min = ~0;
 	best_freq = 0;
@@ -195,9 +195,9 @@ static unsigned int sh_mobile_sdhi_clk_update(struct tmio_mmc_host *host,
 		}
 	}
 
-	clk_set_rate(priv->clk, best_freq);
+	ret = clk_set_rate(priv->clk, best_freq);
 
-	return best_freq;
+	return ret == 0 ? best_freq : clk_get_rate(priv->clk);
 }
 
 static void sh_mobile_sdhi_clk_disable(struct tmio_mmc_host *host)


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

Re: [PATCH v2 2/9] mmc: tmio, sh_mobile_sdhi: Add support for variable input clock frequency

[Wolfram Sang]

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

> > That seems like a bug in the clock driver.  If it doesn't have
> > independent dividers for each clock client then it shouldn't allow any
> > client to change the frequency.

So, we need MSTP clocks which do not use CLK_SET_RATE_PARENT on r8a7740.
Probably the best way is to use the "renesas,r8a7740-mstp-clocks"
compatible entry to determine the difference? Geert?


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

Re: [PATCH v2 2/9] mmc: tmio, sh_mobile_sdhi: Add support for variable input clock frequency

[Wolfram Sang]

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

On Fri, Apr 15, 2016 at 10:41:57PM +0200, Wolfram Sang wrote:
> > > That seems like a bug in the clock driver.  If it doesn't have
> > > independent dividers for each clock client then it shouldn't allow any
> > > client to change the frequency.
> 
> So, we need MSTP clocks which do not use CLK_SET_RATE_PARENT on r8a7740.
> Probably the best way is to use the "renesas,r8a7740-mstp-clocks"
> compatible entry to determine the difference? Geert?

Well, I like the idea of a virtual sd clock even better. I sent a series
doing that a minute ago to the renesas-soc list.


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

Re: [PATCH v2 2/9] mmc: tmio, sh_mobile_sdhi: Add support for variable input clock frequency

[Geert Uytterhoeven]

On Sun, Apr 17, 2016 at 2:49 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> On Fri, Apr 15, 2016 at 10:41:57PM +0200, Wolfram Sang wrote:
>> > > That seems like a bug in the clock driver.  If it doesn't have
>> > > independent dividers for each clock client then it shouldn't allow any
>> > > client to change the frequency.
>>
>> So, we need MSTP clocks which do not use CLK_SET_RATE_PARENT on r8a7740.
>> Probably the best way is to use the "renesas,r8a7740-mstp-clocks"
>> compatible entry to determine the difference? Geert?
>
> Well, I like the idea of a virtual sd clock even better. I sent a series
> doing that a minute ago to the renesas-soc list.

While I don't doubt the "virtual sd" clock will work, this doesn't match the
hardware, so I have to object against it.

Hence I think it should be handled in the driver. This is indeed more difficult
to do with clk-mstp.c than with renesas-cpg-mssr.c...

BTW, this behavior seems to be introduced by
commit e44df332f30bf3040c60c1ed6674d1431fdb48b9
Author: Ben Dooks <ben.dooks@codethink.co.uk>
Date:   Mon Mar 31 18:47:27 2014 +0100

    clk: shmobile: fix setting paretn clock rate

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 v2 2/9] mmc: tmio, sh_mobile_sdhi: Add support for variable input clock frequency

[Geert Uytterhoeven]

Hi Wolfram,

On Fri, Apr 15, 2016 at 10:41 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> > That seems like a bug in the clock driver.  If it doesn't have
>> > independent dividers for each clock client then it shouldn't allow any
>> > client to change the frequency.
>
> So, we need MSTP clocks which do not use CLK_SET_RATE_PARENT on r8a7740.
> Probably the best way is to use the "renesas,r8a7740-mstp-clocks"
> compatible entry to determine the difference? Geert?

To fix this regression in the short term, can we just enable this feature in
the sh_mobile_sdhi driver on SoCs where we know it's safe to change the clock,
i.e. on R-Car Gen2 and H3?

Thanks!

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 v2 2/9] mmc: tmio, sh_mobile_sdhi: Add support for variable input clock frequency

[Wolfram Sang]

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


> To fix this regression in the short term, can we just enable this feature in
> the sh_mobile_sdhi driver on SoCs where we know it's safe to change the clock,
> i.e. on R-Car Gen2 and H3?

I'll try a workaround next week.


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

Re: [PATCH v2 3/9] mmc: tmio: Add UHS-I mode support

[Geert Uytterhoeven]

Hi Wolfram,

On Fri, Apr 1, 2016 at 5:44 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> Based on work by Shinobu Uehara and Ben Dooks. This adds the voltage
> switch operation needed for all UHS-I modes, but not the tuning needed
> for SDR-104 which will come later.
>
> Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

This patch causes a regression on r8a73a4/ape6evm, where the system feels
sluggish, and the load average is always ca. 1.
According to "top", "kworker/0:1" is consuming up to 80% of CPU time.

"echo t > /proc/sysrq-trigger" tells me:

    kworker/0:1     R running      0    57      2 0x00000000
    Workqueue: events_freezable mmc_rescan
    [<c0465e3c>] (__schedule) from [<c04662c4>]
(preempt_schedule_common+0x1c/0x2c)
    [<c04662c4>] (preempt_schedule_common) from [<c0466430>]
(_cond_resched+0x34/0x44)
    [<c0466430>] (_cond_resched) from [<c02f9d04>]
(__mmc_start_request+0x6c/0x204)
    [<c02f9d04>] (__mmc_start_request) from [<c02f9fa0>]
(mmc_start_request+0x104/0x118)
    [<c02f9fa0>] (mmc_start_request) from [<c02fa4b0>]
(mmc_wait_for_req+0x3c/0x14c)
    [<c02fa4b0>] (mmc_wait_for_req) from [<c02fa624>]
(mmc_wait_for_cmd+0x64/0x74)
    [<c02fa624>] (mmc_wait_for_cmd) from [<c03041dc>]
(mmc_io_rw_direct_host+0xbc/0x124)
    [<c03041dc>] (mmc_io_rw_direct_host) from [<c0304608>]
(sdio_reset+0x58/0x60)
    [<c0304608>] (sdio_reset) from [<c02fc4e8>] (mmc_rescan+0x244/0x338)
    [<c02fc4e8>] (mmc_rescan) from [<c00469a8>] (process_one_work+0x324/0x67c)
    [<c00469a8>] (process_one_work) from [<c0046fdc>]
(worker_thread+0x2ac/0x3d4)
    [<c0046fdc>] (worker_thread) from [<c004caf4>] (kthread+0xd8/0xec)
    [<c004caf4>] (kthread) from [<c000fc10>] (ret_from_fork+0x14/0x24)

I've bisected this to
commit 452e5eef6d311e52f657b34d999758107ec3dd4a
Author: Wolfram Sang <wsa+renesas@sang-engineering.com>
Date:   Fri Apr 1 17:44:33 2016 +0200

    mmc: tmio: Add UHS-I mode support

The problem goes away by:
  1. Commenting-out the assignment to .card_busy
  2. OR disabling the second SDHI channel, e.g.
       echo ee120000.sd > /sys/bus/platform/drivers/sh_mobile_sdhi/unbind

The first SDHI channel (ee100000.sd) doesn't seem to be affected
by the problem.

I've added some debug code to dev_warn_ratelimited() the status value.
This shows the SDHI channel keeps on reporting busy:

# dmesg | grep -E "(tmio|sdhi)" | uniq -c
      1 iommu: Adding device regulator-sdhi0 to group 0
      1 iommu: Removing device regulator-sdhi0 from group 0
      1 sh_mobile_sdhi ee100000.sd: could not find pctldev for node
/pfc@e6050000/sd0, deferring probe
      1 sh_mobile_sdhi ee120000.sd: could not find pctldev for node
/pfc@e6050000/sd1, deferring probe
      1 sh_mobile_sdhi ee100000.sd: adding to PM domain a3sp
      1 _host->start_signal_voltage_switch =
sh_mobile_sdhi_start_signal_voltage_switch
      1 sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 max clock
rate 88 MHz
      1 sh_mobile_sdhi ee120000.sd: adding to PM domain a3sp
      1 _host->start_signal_voltage_switch =
sh_mobile_sdhi_start_signal_voltage_switch
      1 sh_mobile_sdhi ee100000.sd: CTL_STATUS2 = 0x20800600
      1 sh_mobile_sdhi ee100000.sd: CTL_STATUS2 = 0x20800400
      1 sh_mobile_sdhi ee120000.sd: mmc1 base at 0xee120000 max clock
rate 12 MHz
      8 sh_mobile_sdhi ee120000.sd: CTL_STATUS2 = 0x20000600
      1 tmio_mmc_card_busy: 3509 callbacks suppressed
     10 sh_mobile_sdhi ee120000.sd: CTL_STATUS2 = 0x20000400
      1 tmio_mmc_card_busy: 2653 callbacks suppressed
     10 sh_mobile_sdhi ee120000.sd: CTL_STATUS2 = 0x20000400
      1 tmio_mmc_card_busy: 2028 callbacks suppressed
     10 sh_mobile_sdhi ee120000.sd: CTL_STATUS2 = 0x20000400
      1 tmio_mmc_card_busy: 2888 callbacks suppressed
     10 sh_mobile_sdhi ee120000.sd: CTL_STATUS2 = 0x20000400

Note that the reported values are the ones for a current tree.
On commit 452e5eef6d311e52, the values are 0x31d2080, 0x3002080,
0x31d2000, and 0x3182000.

As you can see ee100000.sd behaves normal.

> ---
>  drivers/mmc/host/tmio_mmc.h     |  2 ++
>  drivers/mmc/host/tmio_mmc_pio.c | 12 +++++++++++-
>  include/linux/mmc/tmio.h        |  2 ++
>  3 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index b44b5890290622..b1819c74965b47 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -101,6 +101,8 @@ struct tmio_mmc_host {
>         void (*clk_disable)(struct tmio_mmc_host *host);
>         int (*multi_io_quirk)(struct mmc_card *card,
>                               unsigned int direction, int blk_size);
> +       int (*start_signal_voltage_switch)(struct mmc_host *mmc,
> +                                          struct mmc_ios *ios);
>  };
>
>  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 ae81b34f17a5a5..53e5ba5a21914c 100644
> --- a/drivers/mmc/host/tmio_mmc_pio.c
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -1012,12 +1012,20 @@ static int tmio_multi_io_quirk(struct mmc_card *card,
>         return blk_size;
>  }
>
> -static const struct mmc_host_ops tmio_mmc_ops = {
> +static int tmio_mmc_card_busy(struct mmc_host *mmc)
> +{
> +       struct tmio_mmc_host *host = mmc_priv(mmc);
> +
> +       return !(sd_ctrl_read32(host, CTL_STATUS2) & TMIO_STATUS2_DAT0);
> +}
> +
> +static struct mmc_host_ops tmio_mmc_ops = {
>         .request        = tmio_mmc_request,
>         .set_ios        = tmio_mmc_set_ios,
>         .get_ro         = tmio_mmc_get_ro,
>         .get_cd         = mmc_gpio_get_cd,
>         .enable_sdio_irq = tmio_mmc_enable_sdio_irq,
> +       .card_busy      = tmio_mmc_card_busy,
>         .multi_io_quirk = tmio_multi_io_quirk,
>  };
>
> @@ -1116,7 +1124,9 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host,
>                 goto host_free;
>         }
>
> +       tmio_mmc_ops.start_signal_voltage_switch = _host->start_signal_voltage_switch;
>         mmc->ops = &tmio_mmc_ops;
> +
>         mmc->caps |= MMC_CAP_4_BIT_DATA | pdata->capabilities;
>         mmc->caps2 |= pdata->capabilities2;
>         mmc->max_segs = 32;
> diff --git a/include/linux/mmc/tmio.h b/include/linux/mmc/tmio.h
> index 5f5cd80e976500..b2f28e99503383 100644
> --- a/include/linux/mmc/tmio.h
> +++ b/include/linux/mmc/tmio.h
> @@ -63,6 +63,8 @@
>  #define TMIO_STAT_CMD_BUSY      0x40000000
>  #define TMIO_STAT_ILL_ACCESS    0x80000000
>
> +#define TMIO_STATUS2_DAT0      BIT(7)
> +
>  #define        CLK_CTL_DIV_MASK        0xff
>  #define        CLK_CTL_SCLKEN          BIT(8)

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 v2 3/9] mmc: tmio: Add UHS-I mode support

[Wolfram Sang]

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


> The first SDHI channel (ee100000.sd) doesn't seem to be affected
> by the problem.

Hmm, I sadly don't have any docs about 73a4 and/or its SDHI variants.
I'll ask around.


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

Re: [PATCH v2 3/9] mmc: tmio: Add UHS-I mode support

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, Aug 16, 2016 at 9:55 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> The first SDHI channel (ee100000.sd) doesn't seem to be affected
>> by the problem.
>
> Hmm, I sadly don't have any docs about 73a4 and/or its SDHI variants.
> I'll ask around.

Forgot to add: I don't see the issue on sh73a0/kzm9g nor r8a7740/armadillo.

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 v2 3/9] mmc: tmio: Add UHS-I mode support

[Wolfram Sang]

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


>     [<c02f9d04>] (__mmc_start_request) from [<c02f9fa0>]
> (mmc_start_request+0x104/0x118)

This function calls card_busy only for SDIO cards. I assume you don't
have one inserted? Maybe the "broken-cd" property doesn't work correctly
and causes side-effects? This would explain why it only affects SDHI1
and not SDHI0.


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

Re: [PATCH v2 3/9] mmc: tmio: Add UHS-I mode support

[Geert Uytterhoeven]

Hi Wolfram,

On Wed, Aug 17, 2016 at 9:08 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>>     [<c02f9d04>] (__mmc_start_request) from [<c02f9fa0>]
>> (mmc_start_request+0x104/0x118)
>
> This function calls card_busy only for SDIO cards. I assume you don't
> have one inserted? Maybe the "broken-cd" property doesn't work correctly

No, nothing is inserted.

> and causes side-effects? This would explain why it only affects SDHI1
> and not SDHI0.

I'll try to remove it. Note that sh73a0-kzm9g.dts also uses broken-cd,
without ill effects.

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 v2 3/9] mmc: tmio: Add UHS-I mode support

[Wolfram Sang]

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


> I'll try to remove it. Note that sh73a0-kzm9g.dts also uses broken-cd,
> without ill effects.

This one does have a seperate card-detect-irq (which is not populated
for sdhi2) while the other one only has one, generic irq. That's at
least a difference.

We could try to see if there are unexpected card-detect events by
instrumenting mmc_detect_change() in tmio_mmc_pio.c, but my feeling is
that the card_busy change you reverted is only making something visible
which went wrong before the change, also.

So, it might be worth to protect the whole voltage_switch and card_busy
functionality with TMIO_MMC_MIN_RCAR2?


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

Re: [PATCH v2 3/9] mmc: tmio: Add UHS-I mode support

[Geert Uytterhoeven]

Hi Wolfram,

On Wed, Aug 17, 2016 at 9:08 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>>     [<c02f9d04>] (__mmc_start_request) from [<c02f9fa0>]
>> (mmc_start_request+0x104/0x118)
>
> This function calls card_busy only for SDIO cards. I assume you don't
> have one inserted? Maybe the "broken-cd" property doesn't work correctly
> and causes side-effects? This would explain why it only affects SDHI1
> and not SDHI0.

Removing the "broken-cd" property fixes the issue.
However, I'm not in a position to verify SDHI1 works after that.

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