[RFC 0/7] r8a7790: add UHS-I (SDR50) support to Lager

[RFC 0/7] r8a7790: add UHS-I (SDR50) support to Lager

[Wolfram Sang]

So, I rebased and updated the series from Ben to latest renesas-driver and made
it run on my Lager board. I verified that the voltage switching was done. The
transfer speed of my UHS-I card went from 10 MB/s up to 29 MB/s which is the
upper limit of this card. Will try a faster one next week.

The SDHI patches only needed slight modifications due to the rebase. The PFC
patch needed some more rework, but luckily things have gotten easier since back
then, because we can reuse new macros added for bias support. There is one
question regarding this patch, see the comment there. This question is the
reason why the series is RFC because I'd like to hear your opinions there.

Regarding the MMC subsystem, I'd like to ask if it is okay to name the second
pinctrl binding (for uhs) simply "1v8". Is this name driver specific or shall
we aim for a generic name which is used amongst all drivers?

Thanks and happy hacking,

   Wolfram


Ben Hutchings (6):
  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
  mmc: tmio: Add UHS-I mode support
  mmc: sh_mobile_sdhi: Add UHS-I mode support
  ARM: shmobile: r8a7790: Set maximum frequencies for SDHI clocks
  ARM: shmobile: r8a7790: lager: Enable UHS-I SDR-50

Wolfram Sang (1):
  pinctrl: sh-pfc: r8a7790: Implement voltage switching for SDHI

 arch/arm/boot/dts/r8a7790-lager.dts  |  22 ++++++-
 arch/arm/boot/dts/r8a7790.dtsi       |   4 ++
 drivers/mmc/host/sh_mobile_sdhi.c    | 123 ++++++++++++++++++++++++++++++++---
 drivers/mmc/host/tmio_mmc.h          |   9 ++-
 drivers/mmc/host/tmio_mmc_pio.c      |  57 +++++++++++-----
 drivers/pinctrl/sh-pfc/core.c        |   2 +-
 drivers/pinctrl/sh-pfc/core.h        |   1 +
 drivers/pinctrl/sh-pfc/pfc-r8a7790.c |  60 ++++++++++++++++-
 8 files changed, 249 insertions(+), 29 deletions(-)

-- 
2.7.0

[RFC 1/7] pinctrl: sh-pfc: r8a7790: Implement voltage switching for SDHI

[Wolfram Sang]

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

All the SHDIs can operate with either 3.3V or 1.8V signals, depending
on negotiation with the card.

Implement the {get,set}_io_voltage operations and set the related
capability flag for the associated pins.

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: Ulrich Hecht <ulrich.hecht@gmail.com>
---

The get/set-functions introduced here are the same for all Gen2, except for
the pin-sanity checks at the beginning. Somehow a generic function might be
nice for consistency reasons (I don't care so much about the code savings).
But not sure if it is worth the hazzle.

@Uli, Geert: Since bias support is probably similar, what do you think? Does
it make sense to add gen{2,3}.c?


 drivers/pinctrl/sh-pfc/core.c        |  2 +-
 drivers/pinctrl/sh-pfc/core.h        |  1 +
 drivers/pinctrl/sh-pfc/pfc-r8a7790.c | 60 +++++++++++++++++++++++++++++++++++-
 3 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
index 181ea98a63b7ab..315f9db9f5affe 100644
--- a/drivers/pinctrl/sh-pfc/core.c
+++ b/drivers/pinctrl/sh-pfc/core.c
@@ -88,7 +88,7 @@ static int sh_pfc_map_resources(struct sh_pfc *pfc,
 	return 0;
 }
 
-static void __iomem *sh_pfc_phys_to_virt(struct sh_pfc *pfc, u32 reg)
+void __iomem *sh_pfc_phys_to_virt(struct sh_pfc *pfc, u32 reg)
 {
 	struct sh_pfc_window *window;
 	phys_addr_t address = reg;
diff --git a/drivers/pinctrl/sh-pfc/core.h b/drivers/pinctrl/sh-pfc/core.h
index 62f53b22ae8500..ca041a753a2521 100644
--- a/drivers/pinctrl/sh-pfc/core.h
+++ b/drivers/pinctrl/sh-pfc/core.h
@@ -59,6 +59,7 @@ int sh_pfc_unregister_gpiochip(struct sh_pfc *pfc);
 int sh_pfc_register_pinctrl(struct sh_pfc *pfc);
 int sh_pfc_unregister_pinctrl(struct sh_pfc *pfc);
 
+void __iomem *sh_pfc_phys_to_virt(struct sh_pfc *pfc, u32 address);
 u32 sh_pfc_read_raw_reg(void __iomem *mapped_reg, unsigned int reg_width);
 void sh_pfc_write_raw_reg(void __iomem *mapped_reg, unsigned int reg_width,
 			  u32 data);
diff --git a/drivers/pinctrl/sh-pfc/pfc-r8a7790.c b/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
index 0f4d48f9400ba0..ed51d67609050d 100644
--- a/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
+++ b/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
@@ -21,16 +21,21 @@
  * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
  */
 
+#include <linux/io.h>
 #include <linux/kernel.h>
 
 #include "core.h"
 #include "sh_pfc.h"
 
+/*
+ * All pins assigned to GPIO bank 3 can be used for SD interfaces in
+ * which case they support both 3.3V and 1.8V signalling.
+ */
 #define CPU_ALL_PORT(fn, sfx)						\
 	PORT_GP_32(0, fn, sfx),						\
 	PORT_GP_30(1, fn, sfx),						\
 	PORT_GP_30(2, fn, sfx),						\
-	PORT_GP_32(3, fn, sfx),						\
+	PORT_GP_CFG_32(3, fn, sfx, SH_PFC_PIN_CFG_IO_VOLTAGE),		\
 	PORT_GP_32(4, fn, sfx),						\
 	PORT_GP_32(5, fn, sfx)
 
@@ -4691,6 +4696,53 @@ static const char * const vin3_groups[] = {
 	"vin3_clk",
 };
 
+#define IOCTRL6 0xe606008c
+
+static int r8a7790_get_io_voltage(struct sh_pfc *pfc, unsigned int pin)
+{
+	void __iomem *reg;
+	u32 data, mask;
+
+	if (WARN(pin < RCAR_GP_PIN(3, 0) || pin > RCAR_GP_PIN(3, 31), "invalid pin %#x", pin))
+		return -EINVAL;
+
+	reg = sh_pfc_phys_to_virt(pfc, IOCTRL6);
+	data = ioread32(reg);
+
+	/* Bits in IOCTRL6 are numbered in opposite order to pins */
+	mask = 0x80000000 >> (pin & 0x1f);
+
+	return (data & mask) ? 3300 : 1800;
+}
+
+static int r8a7790_set_io_voltage(struct sh_pfc *pfc, unsigned int pin, u16 mV)
+{
+	void __iomem *reg;
+	u32 data, mask;
+
+	if (WARN(pin < RCAR_GP_PIN(3, 0) || pin > RCAR_GP_PIN(3, 31), "invalid pin %#x", pin))
+		return -EINVAL;
+
+	if (mV != 1800 && mV != 3300)
+		return -EINVAL;
+
+	reg = sh_pfc_phys_to_virt(pfc, IOCTRL6);
+	data = ioread32(reg);
+
+	/* Bits in IOCTRL6 are numbered in opposite order to pins */
+	mask = 0x80000000 >> (pin & 0x1f);
+
+	if (mV == 3300)
+		data |= mask;
+	else
+		data &= ~mask;
+
+	iowrite32(~data, sh_pfc_phys_to_virt(pfc, pfc->info->unlock_reg));
+	iowrite32(data, reg);
+
+	return 0;
+}
+
 static const struct sh_pfc_function pinmux_functions[] = {
 	SH_PFC_FUNCTION(audio_clk),
 	SH_PFC_FUNCTION(avb),
@@ -5690,8 +5742,14 @@ static const struct pinmux_cfg_reg pinmux_config_regs[] = {
 	{ },
 };
 
+static const struct sh_pfc_soc_operations pinmux_ops = {
+	.get_io_voltage = r8a7790_get_io_voltage,
+	.set_io_voltage = r8a7790_set_io_voltage,
+};
+
 const struct sh_pfc_soc_info r8a7790_pinmux_info = {
 	.name = "r8a77900_pfc",
+	.ops = &pinmux_ops,
 	.unlock_reg = 0xe6060000, /* PMMR */
 
 	.function = { PINMUX_FUNCTION_BEGIN, PINMUX_FUNCTION_END },
-- 
2.7.0

[RFC 2/7] 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 f7eff5f53e0013..675562653fdbe6 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

[RFC 3/7] 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 675562653fdbe6..4e2ac0e5e5d415 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

[RFC 4/7] mmc: tmio: Add UHS-I mode support

[Wolfram Sang]

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

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.

The card_busy implementation is a bit of a guess, but works for me on
an R8A7790 chip.

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     |  3 +++
 drivers/mmc/host/tmio_mmc_pio.c | 31 +++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index b44b5890290622..aabd36955e73fb 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -101,6 +101,9 @@ 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 tmio_mmc_host *host,
+					   unsigned char signal_voltage);
 };
 
 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..081a3def9305c1 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -1012,12 +1012,43 @@ static int tmio_multi_io_quirk(struct mmc_card *card,
 	return blk_size;
 }
 
+static int tmio_mmc_start_signal_voltage_switch(struct mmc_host *mmc,
+	struct mmc_ios *ios)
+{
+	struct tmio_mmc_host *host = mmc_priv(mmc);
+
+	if (!host->start_signal_voltage_switch)
+		return 0;
+
+	return host->start_signal_voltage_switch(host, ios->signal_voltage);
+}
+
+static int tmio_mmc_card_busy(struct mmc_host *mmc)
+{
+	struct tmio_mmc_host *host = mmc_priv(mmc);
+	u32 status;
+
+	pm_runtime_get_sync(mmc_dev(mmc));
+	status = sd_ctrl_read32(host, CTL_STATUS);
+	pm_runtime_mark_last_busy(mmc_dev(mmc));
+	pm_runtime_put_autosuspend(mmc_dev(mmc));
+
+	/*
+	 * Card signals busy by pulling down all of DAT[3:0].  This status
+	 * flag appears to reflect DAT3.
+	 */
+	return !(status & TMIO_STAT_SIGSTATE_A);
+}
+
 static const 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,
+	.start_signal_voltage_switch
+			= tmio_mmc_start_signal_voltage_switch,
+	.card_busy	= tmio_mmc_card_busy,
 	.multi_io_quirk	= tmio_multi_io_quirk,
 };
 
-- 
2.7.0

[RFC 5/7] mmc: sh_mobile_sdhi: Add UHS-I mode support

[Wolfram Sang]

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

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>
---
 drivers/mmc/host/sh_mobile_sdhi.c | 57 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 4e2ac0e5e5d415..d044fed2736a15 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 *pinctrl_3v3, *pinctrl_1v8;
 };
 
 static void sh_mobile_sdhi_sdbuf_width(struct tmio_mmc_host *host, int width)
@@ -205,6 +210,49 @@ 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 tmio_mmc_host *host, unsigned char signal_voltage)
+{
+	struct sh_mobile_sdhi *priv = host_to_priv(host);
+	struct pinctrl_state *state;
+	int min_uV, max_uV;
+	int ret;
+
+	switch (signal_voltage) {
+	case MMC_SIGNAL_VOLTAGE_330:
+		min_uV = 2700000;
+		max_uV = 3600000;
+		state = priv->pinctrl_3v3;
+		break;
+	case MMC_SIGNAL_VOLTAGE_180:
+		min_uV = 1700000;
+		max_uV = 1950000;
+		state = priv->pinctrl_1v8;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/*
+	 * If anything is missing, assume signal voltage is fixed at
+	 * 3.3V and succeed/fail accordingly.
+	 */
+	if (IS_ERR(host->mmc->supply.vqmmc) ||
+	    IS_ERR(priv->pinctrl) || IS_ERR(state))
+		return signal_voltage == MMC_SIGNAL_VOLTAGE_330 ? 0 : -EINVAL;
+
+	ret = regulator_set_voltage(host->mmc->supply.vqmmc, min_uV, max_uV);
+	if (ret)
+		return ret;
+
+	ret = pinctrl_select_state(priv->pinctrl, 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 +344,14 @@ 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->pinctrl_3v3 = pinctrl_lookup_state(priv->pinctrl,
+							 PINCTRL_STATE_DEFAULT);
+		priv->pinctrl_1v8 = pinctrl_lookup_state(priv->pinctrl,
+							 "1v8");
+	}
+
 	host = tmio_mmc_host_alloc(pdev);
 	if (!host) {
 		ret = -ENOMEM;
@@ -319,6 +375,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

[RFC 6/7] 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 c9583fa6cae713..16cee3f02efb94 100644
--- a/arch/arm/boot/dts/r8a7790.dtsi
+++ b/arch/arm/boot/dts/r8a7790.dtsi
@@ -545,6 +545,7 @@
 		reg = <0 0xee100000 0 0x328>;
 		interrupts = <GIC_SPI 165 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&mstp3_clks R8A7790_CLK_SDHI0>;
+		max-frequency = <156000000>;
 		dmas = <&dmac1 0xcd>, <&dmac1 0xce>;
 		dma-names = "tx", "rx";
 		power-domains = <&cpg_clocks>;
@@ -556,6 +557,7 @@
 		reg = <0 0xee120000 0 0x328>;
 		interrupts = <GIC_SPI 166 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&mstp3_clks R8A7790_CLK_SDHI1>;
+		max-frequency = <156000000>;
 		dmas = <&dmac1 0xc9>, <&dmac1 0xca>;
 		dma-names = "tx", "rx";
 		power-domains = <&cpg_clocks>;
@@ -567,6 +569,7 @@
 		reg = <0 0xee140000 0 0x100>;
 		interrupts = <GIC_SPI 167 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&mstp3_clks R8A7790_CLK_SDHI2>;
+		max-frequency = <97500000>;
 		dmas = <&dmac1 0xc1>, <&dmac1 0xc2>;
 		dma-names = "tx", "rx";
 		power-domains = <&cpg_clocks>;
@@ -578,6 +581,7 @@
 		reg = <0 0xee160000 0 0x100>;
 		interrupts = <GIC_SPI 168 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&mstp3_clks R8A7790_CLK_SDHI3>;
+		max-frequency = <97500000>;
 		dmas = <&dmac1 0xd3>, <&dmac1 0xd4>;
 		dma-names = "tx", "rx";
 		power-domains = <&cpg_clocks>;
-- 
2.7.0

[RFC 7/7] ARM: shmobile: r8a7790: lager: Enable UHS-I SDR-50

[Wolfram Sang]

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

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 cdc0414f5f0716..08eef61ffad1af 100644
--- a/arch/arm/boot/dts/r8a7790-lager.dts
+++ b/arch/arm/boot/dts/r8a7790-lager.dts
@@ -327,11 +327,25 @@
 	sdhi0_pins: sd0 {
 		renesas,groups = "sdhi0_data4", "sdhi0_ctrl";
 		renesas,function = "sdhi0";
+		power-source = <3300>;
+	};
+
+	sdhi0_pins_1v8: sd0_1v8 {
+		renesas,groups = "sdhi0_data4", "sdhi0_ctrl";
+		renesas,function = "sdhi0";
+		power-source = <1800>;
 	};
 
 	sdhi2_pins: sd2 {
 		renesas,groups = "sdhi2_data4", "sdhi2_ctrl";
 		renesas,function = "sdhi2";
+		power-source = <3300>;
+	};
+
+	sdhi2_pins_1v8: sd2_1v8 {
+		renesas,groups = "sdhi2_data4", "sdhi2_ctrl";
+		renesas,function = "sdhi2";
+		power-source = <1800>;
 	};
 
 	mmc1_pins: mmc1 {
@@ -515,21 +529,25 @@
 
 &sdhi0 {
 	pinctrl-0 = <&sdhi0_pins>;
-	pinctrl-names = "default";
+	pinctrl-1 = <&sdhi0_pins_1v8>;
+	pinctrl-names = "default", "1v8";
 
 	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_1v8>;
+	pinctrl-names = "default", "1v8";
 
 	vmmc-supply = <&vcc_sdhi2>;
 	vqmmc-supply = <&vccq_sdhi2>;
 	cd-gpios = <&gpio3 22 GPIO_ACTIVE_LOW>;
+	sd-uhs-sdr50;
 	status = "okay";
 };
 
-- 
2.7.0

Re: [RFC 1/7] pinctrl: sh-pfc: r8a7790: Implement voltage switching for SDHI

[Geert Uytterhoeven]

Hi Wolfram,

On Fri, Feb 19, 2016 at 9:16 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> All the SHDIs can operate with either 3.3V or 1.8V signals, depending
> on negotiation with the card.
>
> Implement the {get,set}_io_voltage operations and set the related
> capability flag for the associated pins.
>
> Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>

Shouldn't the "From" line have Ben's authorship, too?
You can list your modifications in "[...]".

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

Thanks!

> Cc: Ulrich Hecht <ulrich.hecht@gmail.com>
> ---
>
> The get/set-functions introduced here are the same for all Gen2, except for
> the pin-sanity checks at the beginning. Somehow a generic function might be
> nice for consistency reasons (I don't care so much about the code savings).
> But not sure if it is worth the hazzle.
>
> @Uli, Geert: Since bias support is probably similar, what do you think? Does
> it make sense to add gen{2,3}.c?

It's up to you.
We can always consolidate later.

>  drivers/pinctrl/sh-pfc/core.c        |  2 +-
>  drivers/pinctrl/sh-pfc/core.h        |  1 +
>  drivers/pinctrl/sh-pfc/pfc-r8a7790.c | 60 +++++++++++++++++++++++++++++++++++-
>  3 files changed, 61 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
> index 181ea98a63b7ab..315f9db9f5affe 100644
> --- a/drivers/pinctrl/sh-pfc/core.c
> +++ b/drivers/pinctrl/sh-pfc/core.c
> @@ -88,7 +88,7 @@ static int sh_pfc_map_resources(struct sh_pfc *pfc,
>         return 0;
>  }
>
> -static void __iomem *sh_pfc_phys_to_virt(struct sh_pfc *pfc, u32 reg)
> +void __iomem *sh_pfc_phys_to_virt(struct sh_pfc *pfc, u32 reg)

You don't need to make sh_pfc_phys_to_virt() non-static...

>  {
>         struct sh_pfc_window *window;
>         phys_addr_t address = reg;
> diff --git a/drivers/pinctrl/sh-pfc/core.h b/drivers/pinctrl/sh-pfc/core.h
> index 62f53b22ae8500..ca041a753a2521 100644
> --- a/drivers/pinctrl/sh-pfc/core.h
> +++ b/drivers/pinctrl/sh-pfc/core.h
> @@ -59,6 +59,7 @@ int sh_pfc_unregister_gpiochip(struct sh_pfc *pfc);
>  int sh_pfc_register_pinctrl(struct sh_pfc *pfc);
>  int sh_pfc_unregister_pinctrl(struct sh_pfc *pfc);
>
> +void __iomem *sh_pfc_phys_to_virt(struct sh_pfc *pfc, u32 address);

... and declare it...

>  u32 sh_pfc_read_raw_reg(void __iomem *mapped_reg, unsigned int reg_width);
>  void sh_pfc_write_raw_reg(void __iomem *mapped_reg, unsigned int reg_width,
>                           u32 data);
> diff --git a/drivers/pinctrl/sh-pfc/pfc-r8a7790.c b/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
> index 0f4d48f9400ba0..ed51d67609050d 100644
> --- a/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
> +++ b/drivers/pinctrl/sh-pfc/pfc-r8a7790.c

> @@ -4691,6 +4696,53 @@ static const char * const vin3_groups[] = {
>         "vin3_clk",
>  };
>
> +#define IOCTRL6 0xe606008c

... if you use 0x8c here...

> +static int r8a7790_get_io_voltage(struct sh_pfc *pfc, unsigned int pin)
> +{
> +       void __iomem *reg;
> +       u32 data, mask;
> +
> +       if (WARN(pin < RCAR_GP_PIN(3, 0) || pin > RCAR_GP_PIN(3, 31), "invalid pin %#x", pin))
> +               return -EINVAL;
> +
> +       reg = sh_pfc_phys_to_virt(pfc, IOCTRL6);

... and use "pfc->windows->virt + IOCTRL6" here and in
r8a7790_set_io_voltage(), cfr. r8a7778_pinmux_get_bias().

> +       data = ioread32(reg);
> +
> +       /* Bits in IOCTRL6 are numbered in opposite order to pins */
> +       mask = 0x80000000 >> (pin & 0x1f);
> +
> +       return (data & mask) ? 3300 : 1800;
> +}

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: [RFC 1/7] pinctrl: sh-pfc: r8a7790: Implement voltage switching for SDHI

[Wolfram Sang]

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

> Shouldn't the "From" line have Ben's authorship, too?

I considered the changes to this patch significant enough to take over
ownership. Even more so with the changes you suggested.

> > @Uli, Geert: Since bias support is probably similar, what do you think? Does
> > it make sense to add gen{2,3}.c?
> 
> It's up to you.
> We can always consolidate later.

Then let's start like this and see later if consolidation is really needed.

> > @@ -4691,6 +4696,53 @@ static const char * const vin3_groups[] = {
> >         "vin3_clk",
> >  };
> >
> > +#define IOCTRL6 0xe606008c
> 
> ... if you use 0x8c here...
> 
> > +static int r8a7790_get_io_voltage(struct sh_pfc *pfc, unsigned int pin)
> > +{
> > +       void __iomem *reg;
> > +       u32 data, mask;
> > +
> > +       if (WARN(pin < RCAR_GP_PIN(3, 0) || pin > RCAR_GP_PIN(3, 31), "invalid pin %#x", pin))
> > +               return -EINVAL;
> > +
> > +       reg = sh_pfc_phys_to_virt(pfc, IOCTRL6);
> 
> ... and use "pfc->windows->virt + IOCTRL6" here and in
> r8a7790_set_io_voltage(), cfr. r8a7778_pinmux_get_bias().

I see. I thought sh_pfc_phys_to_virt was more correct because it
iterates over all possible windows. But yeah, we can probably live with
the simpler approach on Gen2/3. Will change.

Thanks for the review,

   Wolfram


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

Re: [RFC 1/7] pinctrl: sh-pfc: r8a7790: Implement voltage switching for SDHI

[Wolfram Sang]

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


> ... and use "pfc->windows->virt + IOCTRL6" here and in
> r8a7790_set_io_voltage(), cfr. r8a7778_pinmux_get_bias().

This would mean I need to do something like this for unlocking:

	iowrite32(~data, pfc->windows->virt); /* unlock reg */

I hope this is okay, too.

Note: The new fast SDXC card I got produces timeouts. I want to
investigate first before resending this series.


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

Re: [RFC 4/7] mmc: tmio: Add UHS-I mode support

[Ulf Hansson]

On 19 February 2016 at 21:16, Wolfram Sang <wsa@the-dreams.de> wrote:
> From: Ben Hutchings <ben.hutchings@codethink.co.uk>
>
> 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.
>
> The card_busy implementation is a bit of a guess, but works for me on
> an R8A7790 chip.
>
> 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     |  3 +++
>  drivers/mmc/host/tmio_mmc_pio.c | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+)
>
> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index b44b5890290622..aabd36955e73fb 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -101,6 +101,9 @@ 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 tmio_mmc_host *host,
> +                                          unsigned char signal_voltage);

Do you really need to add a new tmio specific callback for this?

Why can't you instead have the tmio variant driver assign the
->start_signal_voltage_switch() callback into the struct mmc_host_ops
instead?

>  };
>
>  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..081a3def9305c1 100644
> --- a/drivers/mmc/host/tmio_mmc_pio.c
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -1012,12 +1012,43 @@ static int tmio_multi_io_quirk(struct mmc_card *card,
>         return blk_size;
>  }
>
> +static int tmio_mmc_start_signal_voltage_switch(struct mmc_host *mmc,
> +       struct mmc_ios *ios)
> +{
> +       struct tmio_mmc_host *host = mmc_priv(mmc);
> +
> +       if (!host->start_signal_voltage_switch)
> +               return 0;
> +
> +       return host->start_signal_voltage_switch(host, ios->signal_voltage);
> +}
> +
> +static int tmio_mmc_card_busy(struct mmc_host *mmc)
> +{
> +       struct tmio_mmc_host *host = mmc_priv(mmc);
> +       u32 status;
> +
> +       pm_runtime_get_sync(mmc_dev(mmc));

This isn't needed as the mmc core already deal with runtime PM of the
host device via mmc_claim|release_host().

> +       status = sd_ctrl_read32(host, CTL_STATUS);
> +       pm_runtime_mark_last_busy(mmc_dev(mmc));
> +       pm_runtime_put_autosuspend(mmc_dev(mmc));

Ditto.

> +
> +       /*
> +        * Card signals busy by pulling down all of DAT[3:0].  This status
> +        * flag appears to reflect DAT3.
> +        */
> +       return !(status & TMIO_STAT_SIGSTATE_A);
> +}
> +
>  static const 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,
> +       .start_signal_voltage_switch
> +                       = tmio_mmc_start_signal_voltage_switch,
> +       .card_busy      = tmio_mmc_card_busy,
>         .multi_io_quirk = tmio_multi_io_quirk,
>  };
>

Kind regards
Uffe

Re: [RFC 5/7] mmc: sh_mobile_sdhi: Add UHS-I mode support

[Ulf Hansson]

On 19 February 2016 at 21:16, Wolfram Sang <wsa@the-dreams.de> wrote:
> From: Ben Hutchings <ben.hutchings@codethink.co.uk>
>
> 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>
> ---
>  drivers/mmc/host/sh_mobile_sdhi.c | 57 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
>
> diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
> index 4e2ac0e5e5d415..d044fed2736a15 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 *pinctrl_3v3, *pinctrl_1v8;

May I suggest the following names for the pin states instead?
pins_default
pins_uhs

The corresponding pinctrl state-names I prefer are these:

PINCTRL_STATE_DEFAULT and "state_uhs". These names are currently used by mtk-sd.

>  };
>
>  static void sh_mobile_sdhi_sdbuf_width(struct tmio_mmc_host *host, int width)
> @@ -205,6 +210,49 @@ 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 tmio_mmc_host *host, unsigned char signal_voltage)
> +{
> +       struct sh_mobile_sdhi *priv = host_to_priv(host);
> +       struct pinctrl_state *state;
> +       int min_uV, max_uV;
> +       int ret;
> +
> +       switch (signal_voltage) {
> +       case MMC_SIGNAL_VOLTAGE_330:
> +               min_uV = 2700000;
> +               max_uV = 3600000;
> +               state = priv->pinctrl_3v3;
> +               break;
> +       case MMC_SIGNAL_VOLTAGE_180:
> +               min_uV = 1700000;
> +               max_uV = 1950000;
> +               state = priv->pinctrl_1v8;
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       /*
> +        * If anything is missing, assume signal voltage is fixed at
> +        * 3.3V and succeed/fail accordingly.
> +        */
> +       if (IS_ERR(host->mmc->supply.vqmmc) ||
> +           IS_ERR(priv->pinctrl) || IS_ERR(state))
> +               return signal_voltage == MMC_SIGNAL_VOLTAGE_330 ? 0 : -EINVAL;
> +
> +       ret = regulator_set_voltage(host->mmc->supply.vqmmc, min_uV, max_uV);
> +       if (ret)
> +               return ret;

You should make use of the mmc_regulator_set_vqmmc() API here instead.

> +
> +       ret = pinctrl_select_state(priv->pinctrl, 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 +344,14 @@ 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->pinctrl_3v3 = pinctrl_lookup_state(priv->pinctrl,
> +                                                        PINCTRL_STATE_DEFAULT);
> +               priv->pinctrl_1v8 = pinctrl_lookup_state(priv->pinctrl,
> +                                                        "1v8");

I am missing a separate DT binding documentation patch related to the
above pinctrls.

> +       }
> +
>         host = tmio_mmc_host_alloc(pdev);
>         if (!host) {
>                 ret = -ENOMEM;
> @@ -319,6 +375,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
>

Kind regards
Uffe

Re: [RFC 4/7] mmc: tmio: Add UHS-I mode support

[Wolfram Sang]

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

On Thu, Mar 03, 2016 at 03:41:13PM +0100, Ulf Hansson wrote:
> On 19 February 2016 at 21:16, Wolfram Sang <wsa@the-dreams.de> wrote:
> > From: Ben Hutchings <ben.hutchings@codethink.co.uk>
> >
> > 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.
> >
> > The card_busy implementation is a bit of a guess, but works for me on
> > an R8A7790 chip.
> >
> > 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     |  3 +++
> >  drivers/mmc/host/tmio_mmc_pio.c | 31 +++++++++++++++++++++++++++++++
> >  2 files changed, 34 insertions(+)
> >
> > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> > index b44b5890290622..aabd36955e73fb 100644
> > --- a/drivers/mmc/host/tmio_mmc.h
> > +++ b/drivers/mmc/host/tmio_mmc.h
> > @@ -101,6 +101,9 @@ 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 tmio_mmc_host *host,
> > +                                          unsigned char signal_voltage);
> 
> Do you really need to add a new tmio specific callback for this?
> 
> Why can't you instead have the tmio variant driver assign the
> ->start_signal_voltage_switch() callback into the struct mmc_host_ops
> instead?

Can do if you prefer that. I agree that it will save a bit of code; but
it also hides this feature, because all the other config is in struct
tmio_mmc_host. I like the better visibility a tad more.


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

Re: [RFC 4/7] mmc: tmio: Add UHS-I mode support

[Wolfram Sang]

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


> > +
> > +       int (*start_signal_voltage_switch)(struct tmio_mmc_host *host,
> > +                                          unsigned char signal_voltage);
> 
> Do you really need to add a new tmio specific callback for this?
> 
> Why can't you instead have the tmio variant driver assign the
> ->start_signal_voltage_switch() callback into the struct mmc_host_ops
> instead?

Just to see how it looks, I tried your suggestion. However, mmc_ops is
consted all the way into the core (rightfully, I'd say), so this makes
this approach quite clumsy. I kept the above callback now, just changed
the second paramater to pass the whole ios so I can use it with
mmc_regulator_set_vqmmc().

> > +static int tmio_mmc_card_busy(struct mmc_host *mmc)
> > +{
> > +       struct tmio_mmc_host *host = mmc_priv(mmc);
> > +       u32 status;
> > +
> > +       pm_runtime_get_sync(mmc_dev(mmc));
> 
> This isn't needed as the mmc core already deal with runtime PM of the
> host device via mmc_claim|release_host().

sdhci.c does it, too - missing cleanup?


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

Re: [RFC 5/7] mmc: sh_mobile_sdhi: Add UHS-I mode support

[Wolfram Sang]

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


> May I suggest the following names for the pin states instead?
> pins_default
> pins_uhs

Did this.

> The corresponding pinctrl state-names I prefer are these:
> 
> PINCTRL_STATE_DEFAULT and "state_uhs". These names are currently used by mtk-sd.

OK. I'd think "uhs" may be an even better name, because "default"
neither has a "state_" prefix. But for consistency, I'll keep it for now
as "state_uhs".

> I am missing a separate DT binding documentation patch related to the
> above pinctrls.

Was waiting for the bindings to be accepted first :)

Thanks for your reviews!


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

Re: [RFC 4/7] mmc: tmio: Add UHS-I mode support

[Ulf Hansson]

On 9 March 2016 at 17:17, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> > +
>> > +       int (*start_signal_voltage_switch)(struct tmio_mmc_host *host,
>> > +                                          unsigned char signal_voltage);
>>
>> Do you really need to add a new tmio specific callback for this?
>>
>> Why can't you instead have the tmio variant driver assign the
>> ->start_signal_voltage_switch() callback into the struct mmc_host_ops
>> instead?
>
> Just to see how it looks, I tried your suggestion. However, mmc_ops is
> consted all the way into the core (rightfully, I'd say), so this makes
> this approach quite clumsy. I kept the above callback now, just changed
> the second paramater to pass the whole ios so I can use it with
> mmc_regulator_set_vqmmc().

Okay, let me elaborate a bit more on what I really meant.

In tmio_mmc_of_parse(), the mmc->ops gets assigned to the
&tmio_mmc_ops. Before doing that, you can conditionally assign
->start_signal_voltage_switch() callback within the &tmio_mmc_ops,
right!?

Why am I suggesting this? I want to avoid us evolving the code in the
wrong direction, similar what has happened to SDHCI. *One* way to
address this, is to minimize unnecessary variant specific callbacks.

>
>> > +static int tmio_mmc_card_busy(struct mmc_host *mmc)
>> > +{
>> > +       struct tmio_mmc_host *host = mmc_priv(mmc);
>> > +       u32 status;
>> > +
>> > +       pm_runtime_get_sync(mmc_dev(mmc));
>>
>> This isn't needed as the mmc core already deal with runtime PM of the
>> host device via mmc_claim|release_host().
>
> sdhci.c does it, too - missing cleanup?

We quite recently added this into the mmc core, before it was the
controller driver itself who dealt with pm_runtime_get|put().
So, yes several drivers can be cleaned up.

Kind regards
Uffe

Re: [RFC 4/7] mmc: tmio: Add UHS-I mode support

[Wolfram Sang]

Hi Ulf,

> Okay, let me elaborate a bit more on what I really meant.
> 
> In tmio_mmc_of_parse(), the mmc->ops gets assigned to the
> &tmio_mmc_ops. Before doing that, you can conditionally assign
> ->start_signal_voltage_switch() callback within the &tmio_mmc_ops,
> right!?

So, lets talk with code. Do you mean something like this on top of my
series? (Compile tested only to see if I got you right):

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 5d2a852f17eebd..89e0c5e9fcc668 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -210,9 +210,10 @@ 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 tmio_mmc_host *host,
+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;
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 759bf015d09b84..b1819c74965b47 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -101,7 +101,7 @@ 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 tmio_mmc_host *host,
+	int (*start_signal_voltage_switch)(struct mmc_host *mmc,
 					   struct mmc_ios *ios);
 };
 
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index 8dee637638c31e..8e2fef7c5e481c 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -1014,16 +1014,6 @@ static int tmio_multi_io_quirk(struct mmc_card *card,
 	return blk_size;
 }
 
-static int tmio_mmc_start_signal_voltage_switch(struct mmc_host *mmc, struct mmc_ios *ios)
-{
-	struct tmio_mmc_host *host = mmc_priv(mmc);
-
-	if (!host->start_signal_voltage_switch)
-		return 0;
-
-	return host->start_signal_voltage_switch(host, ios);
-}
-
 static int tmio_mmc_card_busy(struct mmc_host *mmc)
 {
 	struct tmio_mmc_host *host = mmc_priv(mmc);
@@ -1031,14 +1021,12 @@ static int tmio_mmc_card_busy(struct mmc_host *mmc)
 	return !(sd_ctrl_read32(host, CTL_STATUS2) & TMIO_STATUS2_DAT0);
 }
 
-static const struct mmc_host_ops tmio_mmc_ops = {
+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,
-	.start_signal_voltage_switch
-			= tmio_mmc_start_signal_voltage_switch,
 	.card_busy	= tmio_mmc_card_busy,
 	.multi_io_quirk	= tmio_multi_io_quirk,
 };
@@ -1138,7 +1126,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;


> Why am I suggesting this? I want to avoid us evolving the code in the
> wrong direction, similar what has happened to SDHCI. *One* way to
> address this, is to minimize unnecessary variant specific callbacks.

Well, I can't test the original tmio_mmc implementation, only the sdhi
variant. So, we need a variant specific start_signal_voltage_switch().
As I see it, the above patch reduces one layer of abstraction. This is
nice, but the variant specific callback is still there. or?

Or i still got you wrong, then sorry!

Thanks,

   Wolfram