[PATCH 0/6] UHS-I support for sh_mobile_sdhi

[PATCH 0/6] UHS-I support for sh_mobile_sdhi

[Ben Hutchings]

This series adds support for UHS-I in sh_mobile_sdhi, partly implemented
in tmio_mmc.  This does not yet include tuning for SDR-104, but SDR-50 now
works on the R8A7790 Lager board and another development board.

The pfc block needs to be reconfigured from 3.3V to 1.8V signalling on
the pins wired to the SD card.  This is supported by adding separate
functions for 1.8V signalling in sh-pfc ("sdhi0_1v8" etc.).  I expect
that several SH SoCs have this capability, but I only have the R8A7790
data sheet so I only implemented it for that one.

Changes since the RFC:
- Replace the 'regulator' devices for signal voltage switching with
  pinctrl functions and states
- Drop 'mmc: sh_mobile_sdhi: Add actual clock rate support' as it's
  redundant
- Use a switch statement in sh_mobile_sdhi_start_signal_voltage_switch()
- Fix subject prefix for the DT changes

Ben.

Ben Dooks (1):
  ARM: shmobile: lager: Set sdhi and mmcif clock rates

Ben Hutchings (5):
  mmc: tmio: Add UHS-I mode support
  pinctrl: sh-pfc: Add set_mux operation to struct sh_pfc_function
  pinctrl: sh-pfc: r8a7790: Add separate functions for SDHI 1.8V
    operation
  mmc: sh_mobile_sdhi: Add UHS-I mode support
  ARM: shmobile: lager: Enable UHS-I SDR-50

 arch/arm/boot/dts/r8a7790-lager.dts  |   29 +++++++++++++-
 drivers/mmc/host/sh_mobile_sdhi.c    |   58 ++++++++++++++++++++++++++++
 drivers/mmc/host/tmio_mmc.h          |    3 ++
 drivers/mmc/host/tmio_mmc_pio.c      |   31 +++++++++++++++
 drivers/pinctrl/sh-pfc/core.c        |    2 +-
 drivers/pinctrl/sh-pfc/core.h        |    1 +
 drivers/pinctrl/sh-pfc/pfc-r8a7790.c |   70 ++++++++++++++++++++++++++++++++--
 drivers/pinctrl/sh-pfc/pinctrl.c     |    4 ++
 drivers/pinctrl/sh-pfc/sh_pfc.h      |   10 ++++-
 9 files changed, 200 insertions(+), 8 deletions(-)

-- 
1.7.10.4

[PATCH 1/6] mmc: tmio: Add UHS-I mode support

[Ben Hutchings]

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.

XXX The card_busy implementation is a bit of a guess.

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
 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 4a597f5a53e2..a1a009879946 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -99,6 +99,9 @@ struct tmio_mmc_host {
 	void (*clk_disable)(struct platform_device *pdev);
 	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 e3dcf31a8bd6..3c1e0e53c0b1 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -1011,12 +1011,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,
 };
 
-- 
1.7.10.4

[PATCH 2/6] pinctrl: sh-pfc: Add set_mux operation to struct sh_pfc_function

[Ben Hutchings]

In some cases a change of function requires changes in more than just
the GPSR/IPSR registers.  In particular, changing the SDHI between 3.3V
and 1.8V signalling also requires modifying IOCTRL6.

Add this optional operation so that such special cases can be handled
in each SoC's code.

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
 drivers/pinctrl/sh-pfc/pinctrl.c |    4 ++++
 drivers/pinctrl/sh-pfc/sh_pfc.h  |   10 +++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/sh-pfc/pinctrl.c b/drivers/pinctrl/sh-pfc/pinctrl.c
index 072e7c62cab7..068b1cdeddde 100644
--- a/drivers/pinctrl/sh-pfc/pinctrl.c
+++ b/drivers/pinctrl/sh-pfc/pinctrl.c
@@ -317,6 +317,7 @@ static int sh_pfc_func_set_mux(struct pinctrl_dev *pctldev, unsigned selector,
 {
 	struct sh_pfc_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
 	struct sh_pfc *pfc = pmx->pfc;
+	const struct sh_pfc_function *func = &pfc->info->functions[selector];
 	const struct sh_pfc_pin_group *grp = &pfc->info->groups[group];
 	unsigned long flags;
 	unsigned int i;
@@ -340,6 +341,9 @@ static int sh_pfc_func_set_mux(struct pinctrl_dev *pctldev, unsigned selector,
 			break;
 	}
 
+	if (func->set_mux)
+		func->set_mux(pfc, func, grp);
+
 done:
 	spin_unlock_irqrestore(&pfc->lock, flags);
 	return ret;
diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h b/drivers/pinctrl/sh-pfc/sh_pfc.h
index c7508d5f6886..0ce4f5c7add8 100644
--- a/drivers/pinctrl/sh-pfc/sh_pfc.h
+++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
@@ -28,6 +28,8 @@ enum {
 #define SH_PFC_PIN_CFG_PULL_DOWN	(1 << 3)
 #define SH_PFC_PIN_CFG_NO_GPIO		(1 << 31)
 
+struct sh_pfc;
+
 struct sh_pfc_pin {
 	u16 pin;
 	u16 enum_id;
@@ -50,17 +52,23 @@ struct sh_pfc_pin_group {
 	unsigned int nr_pins;
 };
 
-#define SH_PFC_FUNCTION(n)				\
+#define SH_PFC_FUNCTION_SPECIAL(n, set_mux_fn)		\
 	{						\
 		.name = #n,				\
 		.groups = n##_groups,			\
 		.nr_groups = ARRAY_SIZE(n##_groups),	\
+		.set_mux = (set_mux_fn),		\
 	}
 
+#define SH_PFC_FUNCTION(n) SH_PFC_FUNCTION_SPECIAL(n, NULL)
+
 struct sh_pfc_function {
 	const char *name;
 	const char * const *groups;
 	unsigned int nr_groups;
+	void (*set_mux)(struct sh_pfc *pfc,
+			const struct sh_pfc_function *func,
+			const struct sh_pfc_pin_group *grp);
 };
 
 struct pinmux_func {
-- 
1.7.10.4

[PATCH 3/6] pinctrl: sh-pfc: r8a7790: Add separate functions for SDHI 1.8V operation

[Ben Hutchings]

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

Add separate functions for the 1.8V mode, and implement the set_mux
operation on all SDHI functions to configure the voltage for each
group of pins.

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
 drivers/pinctrl/sh-pfc/core.c        |    2 +-
 drivers/pinctrl/sh-pfc/core.h        |    1 +
 drivers/pinctrl/sh-pfc/pfc-r8a7790.c |   70 ++++++++++++++++++++++++++++++++--
 3 files changed, 68 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
index 7b2c9495c383..7d51f96afc9a 100644
--- a/drivers/pinctrl/sh-pfc/core.c
+++ b/drivers/pinctrl/sh-pfc/core.c
@@ -92,7 +92,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 6dc8a6fc2746..af355629c5d2 100644
--- a/drivers/pinctrl/sh-pfc/core.h
+++ b/drivers/pinctrl/sh-pfc/core.h
@@ -57,6 +57,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 22a5470889f5..baba3151687f 100644
--- a/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
+++ b/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
@@ -4472,6 +4472,8 @@ static const char * const sdhi0_groups[] = {
 	"sdhi0_wp",
 };
 
+#define sdhi0_1v8_groups sdhi0_groups
+
 static const char * const sdhi1_groups[] = {
 	"sdhi1_data1",
 	"sdhi1_data4",
@@ -4480,6 +4482,8 @@ static const char * const sdhi1_groups[] = {
 	"sdhi1_wp",
 };
 
+#define sdhi1_1v8_groups sdhi1_groups
+
 static const char * const sdhi2_groups[] = {
 	"sdhi2_data1",
 	"sdhi2_data4",
@@ -4488,6 +4492,8 @@ static const char * const sdhi2_groups[] = {
 	"sdhi2_wp",
 };
 
+#define sdhi2_1v8_groups sdhi2_groups
+
 static const char * const sdhi3_groups[] = {
 	"sdhi3_data1",
 	"sdhi3_data4",
@@ -4496,6 +4502,8 @@ static const char * const sdhi3_groups[] = {
 	"sdhi3_wp",
 };
 
+#define sdhi3_1v8_groups sdhi3_groups
+
 static const char * const ssi_groups[] = {
 	"ssi0_data",
 	"ssi0129_ctrl",
@@ -4595,6 +4603,56 @@ static const char * const vin3_groups[] = {
 	"vin3_clk",
 };
 
+static void sdhi_set_voltage(struct sh_pfc *pfc,
+			     const struct sh_pfc_function *func,
+			     const struct sh_pfc_pin_group *grp)
+{
+	void __iomem *mapped_reg;
+	u32 data, mask;
+	int index;
+
+	if (WARN_ON(strncmp(func->name, "sdhi", 4)))
+		return;
+	if (WARN_ON(strncmp(func->name, grp->name, 5)))
+		return;
+
+	/* Calculate mask in IOCTRL6 based on the group */
+	index = func->name[4] - '0';
+	if (WARN_ON(index < 0 || index > 3))
+		return;
+	if (!strcmp(grp->name + 5, "_data1"))
+		mask = 0x20;
+	else if (!strcmp(grp->name + 5, "_data4"))
+		mask = 0x3c;
+	else if (!strcmp(grp->name + 5, "_ctrl"))
+		mask = 0xc0;
+	else if (!strcmp(grp->name + 5, "_cd"))
+		mask = 0x02;
+	else if (!strcmp(grp->name + 5, "_wp"))
+		mask = 0x01;
+	else {
+		WARN_ON(1);
+		return;
+	}
+	mask <<= 8 * (3 - index);
+
+	/* Map IOCTRL6 */
+	mapped_reg = sh_pfc_phys_to_virt(pfc, 0xe606008c);
+
+	data = sh_pfc_read_raw_reg(mapped_reg, 32);
+
+	/* Set for 3.3V (high) or 1.8V (low) depending on the function name */
+	if (strcmp(func->name + 5, "_1v8"))
+		data |= mask;
+	else
+		data &= ~mask;
+
+	sh_pfc_write_raw_reg(
+		sh_pfc_phys_to_virt(pfc, pfc->info->unlock_reg), 32,
+		~data);
+	sh_pfc_write_raw_reg(mapped_reg, 32, data);
+}
+
 static const struct sh_pfc_function pinmux_functions[] = {
 	SH_PFC_FUNCTION(audio_clk),
 	SH_PFC_FUNCTION(avb),
@@ -4631,10 +4689,14 @@ static const struct sh_pfc_function pinmux_functions[] = {
 	SH_PFC_FUNCTION(scifb0),
 	SH_PFC_FUNCTION(scifb1),
 	SH_PFC_FUNCTION(scifb2),
-	SH_PFC_FUNCTION(sdhi0),
-	SH_PFC_FUNCTION(sdhi1),
-	SH_PFC_FUNCTION(sdhi2),
-	SH_PFC_FUNCTION(sdhi3),
+	SH_PFC_FUNCTION_SPECIAL(sdhi0, sdhi_set_voltage),
+	SH_PFC_FUNCTION_SPECIAL(sdhi0_1v8, sdhi_set_voltage),
+	SH_PFC_FUNCTION_SPECIAL(sdhi1, sdhi_set_voltage),
+	SH_PFC_FUNCTION_SPECIAL(sdhi1_1v8, sdhi_set_voltage),
+	SH_PFC_FUNCTION_SPECIAL(sdhi2, sdhi_set_voltage),
+	SH_PFC_FUNCTION_SPECIAL(sdhi2_1v8, sdhi_set_voltage),
+	SH_PFC_FUNCTION_SPECIAL(sdhi3, sdhi_set_voltage),
+	SH_PFC_FUNCTION_SPECIAL(sdhi3_1v8, sdhi_set_voltage),
 	SH_PFC_FUNCTION(ssi),
 	SH_PFC_FUNCTION(tpu0),
 	SH_PFC_FUNCTION(usb0),
-- 
1.7.10.4

[PATCH 4/6] mmc: sh_mobile_sdhi: Add UHS-I mode support

[Ben Hutchings]

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

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 354f4f335ed5..b96d7c084c2e 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -30,6 +30,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"
 
@@ -86,6 +89,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)
@@ -136,6 +141,47 @@ static void sh_mobile_sdhi_clk_disable(struct platform_device *pdev)
 	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;
+
+	if (IS_ERR(priv->pinctrl) || IS_ERR(host->mmc->supply.vqmmc))
+		return -EOPNOTSUPP;
+
+	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 (IS_ERR(state))
+		return PTR_ERR(state);
+
+	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;
@@ -228,6 +274,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;
@@ -239,6 +293,10 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 	host->clk_enable	= sh_mobile_sdhi_clk_enable;
 	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;
+
 	/* SD control register space size is 0x100, 0x200 for bus_shift=1 */
 	if (resource_size(res) > 0x100)
 		host->bus_shift = 1;
-- 
1.7.10.4

Re: [PATCH 4/6] mmc: sh_mobile_sdhi: Add UHS-I mode support

[Kuninori Morimoto]

Hi Ben

> 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>
> ---

I have 1 concern about .start_signal_voltage_switch return value

> +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;
> +
> +	if (IS_ERR(priv->pinctrl) || IS_ERR(host->mmc->supply.vqmmc))
> +		return -EOPNOTSUPP;
(snip)
> @@ -239,6 +293,10 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
>  	host->clk_enable	= sh_mobile_sdhi_clk_enable;
>  	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;
> +

This sh_mobile_sdhi_start_signal_voltage_switch() will return -EOPNOTSUPP
if IS_ERR(xx) cases, and it will be used on .tmio_mmc_start_signal_voltage_switch /
mmc_set_signal_voltage. These will think it is error and goes to error case or try again.
But, not supported is not error ?
Maybe we need this kind of code somewhere ? (ex. ALSA SoC has similar method)

	/* EOPNOTSUPP is not error */
	if (ret == -EOPNOTSUPP)
		ret = 0;

I have no objection if my concern never happen.

Re: [PATCH 4/6] mmc: sh_mobile_sdhi: Add UHS-I mode support

[Ben Hutchings]

On Mon, 2015-05-18 at 01:05 +0000, Kuninori Morimoto wrote:
> Hi Ben
> 
> > 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>
> > ---
> 
> I have 1 concern about .start_signal_voltage_switch return value
> 
> > +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;
> > +
> > +	if (IS_ERR(priv->pinctrl) || IS_ERR(host->mmc->supply.vqmmc))
> > +		return -EOPNOTSUPP;
> (snip)
> > @@ -239,6 +293,10 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
> >  	host->clk_enable	= sh_mobile_sdhi_clk_enable;
> >  	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;
> > +
> 
> This sh_mobile_sdhi_start_signal_voltage_switch() will return -EOPNOTSUPP
> if IS_ERR(xx) cases, and it will be used on .tmio_mmc_start_signal_voltage_switch /
> mmc_set_signal_voltage. These will think it is error and goes to error case or try again.
> But, not supported is not error ?

It is if we need to switch to a lower voltage.

> Maybe we need this kind of code somewhere ? (ex. ALSA SoC has similar method)
> 
> 	/* EOPNOTSUPP is not error */
> 	if (ret == -EOPNOTSUPP)
> 		ret = 0;
> 
> I have no objection if my concern never happen.

I think the driver should do something like this if the requested
voltage is MMC_SIGNAL_VOLTAGE_330.  I'll fix that in the next version.

Ben.

[PATCH 5/6] ARM: shmobile: lager: Set sdhi and mmcif clock rates

[Ben Hutchings]

From: Ben Dooks <ben.dooks@codethink.co.uk>

[bwh: Fold in fix from Ian Molton]
Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
 arch/arm/boot/dts/r8a7790-lager.dts |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
index aaa4f258e279..343ec0ccc8df 100644
--- a/arch/arm/boot/dts/r8a7790-lager.dts
+++ b/arch/arm/boot/dts/r8a7790-lager.dts
@@ -413,6 +413,11 @@
 	vmmc-supply = <&fixedregulator3v3>;
 	bus-width = <8>;
 	non-removable;
+
+	assigned-clocks = <&mstp3_clks R8A7790_CLK_MMCIF1>;
+	assigned-clock-rates = <97500000>;
+	max-frequency = <50000000>;
+
 	status = "okay";
 };
 
@@ -488,6 +493,9 @@
 	pinctrl-0 = <&sdhi0_pins>;
 	pinctrl-names = "default";
 
+	assigned-clocks = <&mstp3_clks R8A7790_CLK_SDHI0>;
+	assigned-clock-rates = <156000000>;
+
 	vmmc-supply = <&vcc_sdhi0>;
 	vqmmc-supply = <&vccq_sdhi0>;
 	cd-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>;
@@ -498,6 +506,9 @@
 	pinctrl-0 = <&sdhi2_pins>;
 	pinctrl-names = "default";
 
+	assigned-clocks = <&mstp3_clks R8A7790_CLK_SDHI2>;
+	assigned-clock-rates = <97500000>;
+
 	vmmc-supply = <&vcc_sdhi2>;
 	vqmmc-supply = <&vccq_sdhi2>;
 	cd-gpios = <&gpio3 22 GPIO_ACTIVE_LOW>;
-- 
1.7.10.4

Re: [PATCH 5/6] ARM: shmobile: lager: Set sdhi and mmcif clock rates

[Geert Uytterhoeven]

On Sun, May 17, 2015 at 2:29 AM, Ben Hutchings
<ben.hutchings@codethink.co.uk> wrote:
> From: Ben Dooks <ben.dooks@codethink.co.uk>
>
> [bwh: Fold in fix from Ian Molton]
> Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> ---
>  arch/arm/boot/dts/r8a7790-lager.dts |   11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
> index aaa4f258e279..343ec0ccc8df 100644
> --- a/arch/arm/boot/dts/r8a7790-lager.dts
> +++ b/arch/arm/boot/dts/r8a7790-lager.dts
> @@ -413,6 +413,11 @@
>         vmmc-supply = <&fixedregulator3v3>;
>         bus-width = <8>;
>         non-removable;
> +
> +       assigned-clocks = <&mstp3_clks R8A7790_CLK_MMCIF1>;
> +       assigned-clock-rates = <97500000>;
> +       max-frequency = <50000000>;

Are all these properties Lager-specific, or r8a7790-specific?
If the latter, they belong in r8a7790.dtsi.
Same comment for the other nodes.

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 5/6] ARM: shmobile: lager: Set sdhi and mmcif clock rates

[Ben Hutchings]

On Sun, 2015-05-17 at 10:13 +0200, Geert Uytterhoeven wrote:
> On Sun, May 17, 2015 at 2:29 AM, Ben Hutchings
> <ben.hutchings@codethink.co.uk> wrote:
> > From: Ben Dooks <ben.dooks@codethink.co.uk>
> >
> > [bwh: Fold in fix from Ian Molton]
> > Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> > ---
> >  arch/arm/boot/dts/r8a7790-lager.dts |   11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
> > index aaa4f258e279..343ec0ccc8df 100644
> > --- a/arch/arm/boot/dts/r8a7790-lager.dts
> > +++ b/arch/arm/boot/dts/r8a7790-lager.dts
> > @@ -413,6 +413,11 @@
> >         vmmc-supply = <&fixedregulator3v3>;
> >         bus-width = <8>;
> >         non-removable;
> > +
> > +       assigned-clocks = <&mstp3_clks R8A7790_CLK_MMCIF1>;
> > +       assigned-clock-rates = <97500000>;
> > +       max-frequency = <50000000>;
> 
> Are all these properties Lager-specific, or r8a7790-specific?
> If the latter, they belong in r8a7790.dtsi.
> Same comment for the other nodes.

The clock assignments are not specific to Lager so they probably belong
in r8a7790.dtsi; the dividers are configurable and could be
board-specific.

The board design could conceivably limit the usable frequency to below
the chip's maximum, though presumably that won't usually happen.
Currently the clk-rcar-gen2 driver only configures the dividers for
sdhi{0,1}.  That leaves the rest of them effectively controlled by the
board's firmware.

Ben.

Re: [PATCH 5/6] ARM: shmobile: lager: Set sdhi and mmcif clock rates

[Ben Hutchings]

On Mon, 2015-05-18 at 20:48 +0100, Ben Hutchings wrote:
> On Sun, 2015-05-17 at 10:13 +0200, Geert Uytterhoeven wrote:
> > On Sun, May 17, 2015 at 2:29 AM, Ben Hutchings
> > <ben.hutchings@codethink.co.uk> wrote:
> > > From: Ben Dooks <ben.dooks@codethink.co.uk>
> > >
> > > [bwh: Fold in fix from Ian Molton]
> > > Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> > > ---
> > >  arch/arm/boot/dts/r8a7790-lager.dts |   11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > >
> > > diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
> > > index aaa4f258e279..343ec0ccc8df 100644
> > > --- a/arch/arm/boot/dts/r8a7790-lager.dts
> > > +++ b/arch/arm/boot/dts/r8a7790-lager.dts
> > > @@ -413,6 +413,11 @@
> > >         vmmc-supply = <&fixedregulator3v3>;
> > >         bus-width = <8>;
> > >         non-removable;
> > > +
> > > +       assigned-clocks = <&mstp3_clks R8A7790_CLK_MMCIF1>;
> > > +       assigned-clock-rates = <97500000>;
> > > +       max-frequency = <50000000>;
> > 
> > Are all these properties Lager-specific, or r8a7790-specific?
> > If the latter, they belong in r8a7790.dtsi.
> > Same comment for the other nodes.
> 
> The clock assignments are not specific to Lager so they probably belong
> in r8a7790.dtsi; the dividers are configurable and could be
> board-specific.

The problem with doing that is I think the assigned-clocks and
assigned-clock-rates properties belong together.  I can't find any other
instance where they are split between a .dtsi file and .dts files that
include it.

I *could* define both properties in r8a7790.dtsi and then let board .dts
files override the clock rate if wanted.

> The board design could conceivably limit the usable frequency to below
> the chip's maximum, though presumably that won't usually happen.
> Currently the clk-rcar-gen2 driver only configures the dividers for
> sdhi{0,1}.  That leaves the rest of them effectively controlled by the
> board's firmware.

I was mistaken about this; the other dividers are programmed by
clk-div6.

Ben.

[PATCH 6/6] ARM: shmobile: lager: Enable UHS-I SDR-50

[Ben Hutchings]

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

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
None of the states includes the CD pins, as they can't be allocated both
through pinctrl and as GPIOs.  But the Lager manual shows these signals
being pulled up to the same variable voltage as the other signals.  This
might possibly lead to spurious card detect interrupts after switching
to 1.8V signalling.

Ben.

 arch/arm/boot/dts/r8a7790-lager.dts |   18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
index 343ec0ccc8df..5584e835d0f5 100644
--- a/arch/arm/boot/dts/r8a7790-lager.dts
+++ b/arch/arm/boot/dts/r8a7790-lager.dts
@@ -314,11 +314,21 @@
 		renesas,function = "sdhi0";
 	};
 
+	sdhi0_pins_1v8: sd0_1v8 {
+		renesas,groups = "sdhi0_data4", "sdhi0_ctrl";
+		renesas,function = "sdhi0_1v8";
+	};
+
 	sdhi2_pins: sd2 {
 		renesas,groups = "sdhi2_data4", "sdhi2_ctrl";
 		renesas,function = "sdhi2";
 	};
 
+	sdhi2_pins_1v8: sd2_1v8 {
+		renesas,groups = "sdhi2_data4", "sdhi2_ctrl";
+		renesas,function = "sdhi2_1v8";
+	};
+
 	mmc1_pins: mmc1 {
 		renesas,groups = "mmc1_data8", "mmc1_ctrl";
 		renesas,function = "mmc1";
@@ -491,7 +501,8 @@
 
 &sdhi0 {
 	pinctrl-0 = <&sdhi0_pins>;
-	pinctrl-names = "default";
+	pinctrl-1 = <&sdhi0_pins_1v8>;
+	pinctrl-names = "default", "1v8";
 
 	assigned-clocks = <&mstp3_clks R8A7790_CLK_SDHI0>;
 	assigned-clock-rates = <156000000>;
@@ -499,12 +510,14 @@
 	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";
 
 	assigned-clocks = <&mstp3_clks R8A7790_CLK_SDHI2>;
 	assigned-clock-rates = <97500000>;
@@ -512,6 +525,7 @@
 	vmmc-supply = <&vcc_sdhi2>;
 	vqmmc-supply = <&vccq_sdhi2>;
 	cd-gpios = <&gpio3 22 GPIO_ACTIVE_LOW>;
+	sd-uhs-sdr50;
 	status = "okay";
 };
 
-- 
1.7.10.4

Re: [PATCH 6/6] ARM: shmobile: lager: Enable UHS-I SDR-50

[Simon Horman]

[CC Morimoto-san]

On Sun, May 17, 2015 at 01:39:44AM +0100, Ben Hutchings wrote:
> Add the "1v8" pinctrl state and sd-uhs-sdr50 property to SDHI{0,2}.
> 
> Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> ---
> None of the states includes the CD pins, as they can't be allocated both
> through pinctrl and as GPIOs.  But the Lager manual shows these signals
> being pulled up to the same variable voltage as the other signals.  This
> might possibly lead to spurious card detect interrupts after switching
> to 1.8V signalling.

Morimoto-san,

I would appreciate your opinion on this change.

Thanks.

>  arch/arm/boot/dts/r8a7790-lager.dts |   18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
> index 343ec0ccc8df..5584e835d0f5 100644
> --- a/arch/arm/boot/dts/r8a7790-lager.dts
> +++ b/arch/arm/boot/dts/r8a7790-lager.dts
> @@ -314,11 +314,21 @@
>  		renesas,function = "sdhi0";
>  	};
>  
> +	sdhi0_pins_1v8: sd0_1v8 {
> +		renesas,groups = "sdhi0_data4", "sdhi0_ctrl";
> +		renesas,function = "sdhi0_1v8";
> +	};
> +
>  	sdhi2_pins: sd2 {
>  		renesas,groups = "sdhi2_data4", "sdhi2_ctrl";
>  		renesas,function = "sdhi2";
>  	};
>  
> +	sdhi2_pins_1v8: sd2_1v8 {
> +		renesas,groups = "sdhi2_data4", "sdhi2_ctrl";
> +		renesas,function = "sdhi2_1v8";
> +	};
> +
>  	mmc1_pins: mmc1 {
>  		renesas,groups = "mmc1_data8", "mmc1_ctrl";
>  		renesas,function = "mmc1";
> @@ -491,7 +501,8 @@
>  
>  &sdhi0 {
>  	pinctrl-0 = <&sdhi0_pins>;
> -	pinctrl-names = "default";
> +	pinctrl-1 = <&sdhi0_pins_1v8>;
> +	pinctrl-names = "default", "1v8";
>  
>  	assigned-clocks = <&mstp3_clks R8A7790_CLK_SDHI0>;
>  	assigned-clock-rates = <156000000>;
> @@ -499,12 +510,14 @@
>  	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";
>  
>  	assigned-clocks = <&mstp3_clks R8A7790_CLK_SDHI2>;
>  	assigned-clock-rates = <97500000>;
> @@ -512,6 +525,7 @@
>  	vmmc-supply = <&vcc_sdhi2>;
>  	vqmmc-supply = <&vccq_sdhi2>;
>  	cd-gpios = <&gpio3 22 GPIO_ACTIVE_LOW>;
> +	sd-uhs-sdr50;
>  	status = "okay";
>  };
>  
> -- 
> 1.7.10.4
> 
> 
> 

Re: [PATCH 6/6] ARM: shmobile: lager: Enable UHS-I SDR-50

[Kuninori Morimoto]

Hi Simon

> On Sun, May 17, 2015 at 01:39:44AM +0100, Ben Hutchings wrote:
> > Add the "1v8" pinctrl state and sd-uhs-sdr50 property to SDHI{0,2}.
> > 
> > Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> > ---
> > None of the states includes the CD pins, as they can't be allocated both
> > through pinctrl and as GPIOs.  But the Lager manual shows these signals
> > being pulled up to the same variable voltage as the other signals.  This
> > might possibly lead to spurious card detect interrupts after switching
> > to 1.8V signalling.
> 
> Morimoto-san,
> 
> I would appreciate your opinion on this change.

Ben will send new version patch set if my understanding is correct


Best regards
---
Kuninori Morimoto

Re: [PATCH 6/6] ARM: shmobile: lager: Enable UHS-I SDR-50

[Simon Horman]

On Mon, May 25, 2015 at 04:00:11AM +0000, Kuninori Morimoto wrote:
> 
> Hi Simon
> 
> > On Sun, May 17, 2015 at 01:39:44AM +0100, Ben Hutchings wrote:
> > > Add the "1v8" pinctrl state and sd-uhs-sdr50 property to SDHI{0,2}.
> > > 
> > > Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> > > ---
> > > None of the states includes the CD pins, as they can't be allocated both
> > > through pinctrl and as GPIOs.  But the Lager manual shows these signals
> > > being pulled up to the same variable voltage as the other signals.  This
> > > might possibly lead to spurious card detect interrupts after switching
> > > to 1.8V signalling.
> > 
> > Morimoto-san,
> > 
> > I would appreciate your opinion on this change.
> 
> Ben will send new version patch set if my understanding is correct

Thanks, got it.