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

[PATCH v3 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 implementing the
pinconf power-source parameter.  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 v2:
- Implement the pinconf power-source parameter instead of separate
  functions for the low-voltage mode
- Adjust low-voltage mode setter function and device tree accordingly
- Add low-voltage mode getter function for R8A7790

Changes since v1:
- Reword commit message for "mmc: tmio: Add UHS-I mode support"
- Make sh_mobile_sdhi_start_signal_voltage_switch() succeed if asked
  to switch to 3.3V and the regulator or pinctrl or pinctrl state is
  missing
- Drop change to mmcif clock on Lager
- Correct original author for sdhi clock changes on Lager

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 Hutchings (5):
  mmc: tmio: Add UHS-I mode support
  pinctrl: sh-pfc: Add low-voltage pinconf parameter
  pinctrl: sh-pfc: r8a7790: Implement voltage switching for SDHI
  mmc: sh_mobile_sdhi: Add UHS-I mode support
  ARM: shmobile: lager: Enable UHS-I SDR-50

Ian Molton (1):
  ARM: shmobile: lager: Set clock rates for SDHI

 .../bindings/pinctrl/renesas,pfc-pinctrl.txt       |  6 +-
 arch/arm/boot/dts/r8a7790-lager.dts                | 28 ++++++++-
 drivers/mmc/host/sh_mobile_sdhi.c                  | 60 +++++++++++++++++++
 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               | 67 +++++++++++++++++++++-
 drivers/pinctrl/sh-pfc/pinctrl.c                   | 57 +++++++++++++++---
 drivers/pinctrl/sh-pfc/sh_pfc.h                    | 11 ++++
 10 files changed, 252 insertions(+), 14 deletions(-)

-- 
2.1.4

[PATCH v3 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 implementing the
pinconf power-source parameter.  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 v2:
- Implement the pinconf power-source parameter instead of separate
  functions for the low-voltage mode
- Adjust low-voltage mode setter function and device tree accordingly
- Add low-voltage mode getter function for R8A7790

Changes since v1:
- Reword commit message for "mmc: tmio: Add UHS-I mode support"
- Make sh_mobile_sdhi_start_signal_voltage_switch() succeed if asked
  to switch to 3.3V and the regulator or pinctrl or pinctrl state is
  missing
- Drop change to mmcif clock on Lager
- Correct original author for sdhi clock changes on Lager

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 Hutchings (5):
  mmc: tmio: Add UHS-I mode support
  pinctrl: sh-pfc: Add low-voltage pinconf parameter
  pinctrl: sh-pfc: r8a7790: Implement voltage switching for SDHI
  mmc: sh_mobile_sdhi: Add UHS-I mode support
  ARM: shmobile: lager: Enable UHS-I SDR-50

Ian Molton (1):
  ARM: shmobile: lager: Set clock rates for SDHI

 .../bindings/pinctrl/renesas,pfc-pinctrl.txt       |  6 +-
 arch/arm/boot/dts/r8a7790-lager.dts                | 28 ++++++++-
 drivers/mmc/host/sh_mobile_sdhi.c                  | 60 +++++++++++++++++++
 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               | 67 +++++++++++++++++++++-
 drivers/pinctrl/sh-pfc/pinctrl.c                   | 57 +++++++++++++++---
 drivers/pinctrl/sh-pfc/sh_pfc.h                    | 11 ++++
 10 files changed, 252 insertions(+), 14 deletions(-)

-- 
2.1.4

[PATCH v3 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.

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>
---
 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,
 };
 
-- 
2.1.4

[PATCH v3 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.

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>
---
 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,
 };
 
-- 
2.1.4

[PATCH v3 2/6] pinctrl: sh-pfc: Implement pinconf power-source param for voltage switching

[Ben Hutchings]

The pfc in the R8A7790 (and probably others in the R-Car gen 2 family)
supports switching SDHI signals between 3.3V and 1.8V voltage, and the
SD driver should do that when selecting a higher-speed mode.

Add a flag for pins that support low voltage mode and SoC operations to
get and set it.  Implement the pinconf power-source parameter using these
operations.

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
 .../bindings/pinctrl/renesas,pfc-pinctrl.txt       |  4 ++-
 drivers/pinctrl/sh-pfc/pinctrl.c                   | 34 ++++++++++++++++++++--
 drivers/pinctrl/sh-pfc/sh_pfc.h                    |  6 ++++
 3 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
index bfe72ec055e3..93651742f1c1 100644
--- a/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
@@ -69,7 +69,9 @@ Pin Configuration Node Properties:
 
 The pin configuration parameters use the generic pinconf bindings defined in
 pinctrl-bindings.txt in this directory. The supported parameters are
-bias-disable, bias-pull-up and bias-pull-down.
+bias-disable, bias-pull-up, bias-pull-down and power-source. For pins that
+can operate with either 3.3V or 1.8V signals, the power-source value should
+be 0 for 3.3V or 1 for 1.8V.
 

 GPIO
diff --git a/drivers/pinctrl/sh-pfc/pinctrl.c b/drivers/pinctrl/sh-pfc/pinctrl.c
index 072e7c62cab7..fa4066d4bc61 100644
--- a/drivers/pinctrl/sh-pfc/pinctrl.c
+++ b/drivers/pinctrl/sh-pfc/pinctrl.c
@@ -465,6 +465,9 @@ static bool sh_pfc_pinconf_validate(struct sh_pfc *pfc, unsigned int _pin,
 	case PIN_CONFIG_BIAS_PULL_DOWN:
 		return pin->configs & SH_PFC_PIN_CFG_PULL_DOWN;
 
+	case PIN_CONFIG_POWER_SOURCE:
+		return pin->configs & SH_PFC_PIN_CFG_LOW_VOLTAGE;
+
 	default:
 		return false;
 	}
@@ -477,7 +480,6 @@ static int sh_pfc_pinconf_get(struct pinctrl_dev *pctldev, unsigned _pin,
 	struct sh_pfc *pfc = pmx->pfc;
 	enum pin_config_param param = pinconf_to_config_param(*config);
 	unsigned long flags;
-	unsigned int bias;
 
 	if (!sh_pfc_pinconf_validate(pfc, _pin, param))
 		return -ENOTSUPP;
@@ -485,7 +487,9 @@ static int sh_pfc_pinconf_get(struct pinctrl_dev *pctldev, unsigned _pin,
 	switch (param) {
 	case PIN_CONFIG_BIAS_DISABLE:
 	case PIN_CONFIG_BIAS_PULL_UP:
-	case PIN_CONFIG_BIAS_PULL_DOWN:
+	case PIN_CONFIG_BIAS_PULL_DOWN: {
+		unsigned int bias;
+
 		if (!pfc->info->ops || !pfc->info->ops->get_bias)
 			return -ENOTSUPP;
 
@@ -498,6 +502,16 @@ static int sh_pfc_pinconf_get(struct pinctrl_dev *pctldev, unsigned _pin,
 
 		*config = 0;
 		break;
+	}
+
+	case PIN_CONFIG_POWER_SOURCE:
+		if (!pfc->info->ops || !pfc->info->ops->get_low_voltage)
+			return -ENOTSUPP;
+
+		spin_lock_irqsave(&pfc->lock, flags);
+		*config = pfc->info->ops->get_low_voltage(pfc, _pin);
+		spin_unlock_irqrestore(&pfc->lock, flags);
+		break;
 
 	default:
 		return -ENOTSUPP;
@@ -534,6 +548,22 @@ static int sh_pfc_pinconf_set(struct pinctrl_dev *pctldev, unsigned _pin,
 
 			break;
 
+		case PIN_CONFIG_POWER_SOURCE: {
+			unsigned int arg +				pinconf_to_config_argument(configs[i]);
+
+			if (!pfc->info->ops || !pfc->info->ops->set_low_voltage)
+				return -ENOTSUPP;
+			if (arg > 1)
+				return -ENOTSUPP;
+
+			spin_lock_irqsave(&pfc->lock, flags);
+			pfc->info->ops->set_low_voltage(pfc, _pin, arg);
+			spin_unlock_irqrestore(&pfc->lock, flags);
+
+			break;
+		}
+
 		default:
 			return -ENOTSUPP;
 		}
diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h b/drivers/pinctrl/sh-pfc/sh_pfc.h
index c7508d5f6886..b95d60bf2f1b 100644
--- a/drivers/pinctrl/sh-pfc/sh_pfc.h
+++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
@@ -13,6 +13,7 @@
 
 #include <linux/bug.h>
 #include <linux/stringify.h>
+#include <linux/pinctrl/pinconf-generic.h>
 
 enum {
 	PINMUX_TYPE_NONE,
@@ -26,8 +27,11 @@ enum {
 #define SH_PFC_PIN_CFG_OUTPUT		(1 << 1)
 #define SH_PFC_PIN_CFG_PULL_UP		(1 << 2)
 #define SH_PFC_PIN_CFG_PULL_DOWN	(1 << 3)
+#define SH_PFC_PIN_CFG_LOW_VOLTAGE	(1 << 4)
 #define SH_PFC_PIN_CFG_NO_GPIO		(1 << 31)
 
+struct sh_pfc;
+
 struct sh_pfc_pin {
 	u16 pin;
 	u16 enum_id;
@@ -121,6 +125,8 @@ struct sh_pfc_soc_operations {
 	unsigned int (*get_bias)(struct sh_pfc *pfc, unsigned int pin);
 	void (*set_bias)(struct sh_pfc *pfc, unsigned int pin,
 			 unsigned int bias);
+	bool (*get_low_voltage)(struct sh_pfc *pfc, unsigned int pin);
+	void (*set_low_voltage)(struct sh_pfc *pfc, unsigned int pin, bool low);
 };
 
 struct sh_pfc_soc_info {
-- 
2.1.4

[PATCH v3 2/6] pinctrl: sh-pfc: Implement pinconf power-source param for voltage switching

[Ben Hutchings]

The pfc in the R8A7790 (and probably others in the R-Car gen 2 family)
supports switching SDHI signals between 3.3V and 1.8V voltage, and the
SD driver should do that when selecting a higher-speed mode.

Add a flag for pins that support low voltage mode and SoC operations to
get and set it.  Implement the pinconf power-source parameter using these
operations.

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
 .../bindings/pinctrl/renesas,pfc-pinctrl.txt       |  4 ++-
 drivers/pinctrl/sh-pfc/pinctrl.c                   | 34 ++++++++++++++++++++--
 drivers/pinctrl/sh-pfc/sh_pfc.h                    |  6 ++++
 3 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
index bfe72ec055e3..93651742f1c1 100644
--- a/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
@@ -69,7 +69,9 @@ Pin Configuration Node Properties:
 
 The pin configuration parameters use the generic pinconf bindings defined in
 pinctrl-bindings.txt in this directory. The supported parameters are
-bias-disable, bias-pull-up and bias-pull-down.
+bias-disable, bias-pull-up, bias-pull-down and power-source. For pins that
+can operate with either 3.3V or 1.8V signals, the power-source value should
+be 0 for 3.3V or 1 for 1.8V.
 

 GPIO
diff --git a/drivers/pinctrl/sh-pfc/pinctrl.c b/drivers/pinctrl/sh-pfc/pinctrl.c
index 072e7c62cab7..fa4066d4bc61 100644
--- a/drivers/pinctrl/sh-pfc/pinctrl.c
+++ b/drivers/pinctrl/sh-pfc/pinctrl.c
@@ -465,6 +465,9 @@ static bool sh_pfc_pinconf_validate(struct sh_pfc *pfc, unsigned int _pin,
 	case PIN_CONFIG_BIAS_PULL_DOWN:
 		return pin->configs & SH_PFC_PIN_CFG_PULL_DOWN;
 
+	case PIN_CONFIG_POWER_SOURCE:
+		return pin->configs & SH_PFC_PIN_CFG_LOW_VOLTAGE;
+
 	default:
 		return false;
 	}
@@ -477,7 +480,6 @@ static int sh_pfc_pinconf_get(struct pinctrl_dev *pctldev, unsigned _pin,
 	struct sh_pfc *pfc = pmx->pfc;
 	enum pin_config_param param = pinconf_to_config_param(*config);
 	unsigned long flags;
-	unsigned int bias;
 
 	if (!sh_pfc_pinconf_validate(pfc, _pin, param))
 		return -ENOTSUPP;
@@ -485,7 +487,9 @@ static int sh_pfc_pinconf_get(struct pinctrl_dev *pctldev, unsigned _pin,
 	switch (param) {
 	case PIN_CONFIG_BIAS_DISABLE:
 	case PIN_CONFIG_BIAS_PULL_UP:
-	case PIN_CONFIG_BIAS_PULL_DOWN:
+	case PIN_CONFIG_BIAS_PULL_DOWN: {
+		unsigned int bias;
+
 		if (!pfc->info->ops || !pfc->info->ops->get_bias)
 			return -ENOTSUPP;
 
@@ -498,6 +502,16 @@ static int sh_pfc_pinconf_get(struct pinctrl_dev *pctldev, unsigned _pin,
 
 		*config = 0;
 		break;
+	}
+
+	case PIN_CONFIG_POWER_SOURCE:
+		if (!pfc->info->ops || !pfc->info->ops->get_low_voltage)
+			return -ENOTSUPP;
+
+		spin_lock_irqsave(&pfc->lock, flags);
+		*config = pfc->info->ops->get_low_voltage(pfc, _pin);
+		spin_unlock_irqrestore(&pfc->lock, flags);
+		break;
 
 	default:
 		return -ENOTSUPP;
@@ -534,6 +548,22 @@ static int sh_pfc_pinconf_set(struct pinctrl_dev *pctldev, unsigned _pin,
 
 			break;
 
+		case PIN_CONFIG_POWER_SOURCE: {
+			unsigned int arg =
+				pinconf_to_config_argument(configs[i]);
+
+			if (!pfc->info->ops || !pfc->info->ops->set_low_voltage)
+				return -ENOTSUPP;
+			if (arg > 1)
+				return -ENOTSUPP;
+
+			spin_lock_irqsave(&pfc->lock, flags);
+			pfc->info->ops->set_low_voltage(pfc, _pin, arg);
+			spin_unlock_irqrestore(&pfc->lock, flags);
+
+			break;
+		}
+
 		default:
 			return -ENOTSUPP;
 		}
diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h b/drivers/pinctrl/sh-pfc/sh_pfc.h
index c7508d5f6886..b95d60bf2f1b 100644
--- a/drivers/pinctrl/sh-pfc/sh_pfc.h
+++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
@@ -13,6 +13,7 @@
 
 #include <linux/bug.h>
 #include <linux/stringify.h>
+#include <linux/pinctrl/pinconf-generic.h>
 
 enum {
 	PINMUX_TYPE_NONE,
@@ -26,8 +27,11 @@ enum {
 #define SH_PFC_PIN_CFG_OUTPUT		(1 << 1)
 #define SH_PFC_PIN_CFG_PULL_UP		(1 << 2)
 #define SH_PFC_PIN_CFG_PULL_DOWN	(1 << 3)
+#define SH_PFC_PIN_CFG_LOW_VOLTAGE	(1 << 4)
 #define SH_PFC_PIN_CFG_NO_GPIO		(1 << 31)
 
+struct sh_pfc;
+
 struct sh_pfc_pin {
 	u16 pin;
 	u16 enum_id;
@@ -121,6 +125,8 @@ struct sh_pfc_soc_operations {
 	unsigned int (*get_bias)(struct sh_pfc *pfc, unsigned int pin);
 	void (*set_bias)(struct sh_pfc *pfc, unsigned int pin,
 			 unsigned int bias);
+	bool (*get_low_voltage)(struct sh_pfc *pfc, unsigned int pin);
+	void (*set_low_voltage)(struct sh_pfc *pfc, unsigned int pin, bool low);
 };
 
 struct sh_pfc_soc_info {
-- 
2.1.4

[PATCH v3 3/6] pinctrl: sh-pfc: r8a7790: Implement voltage switching for SDHI

[Ben Hutchings]

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

Implement the {get,set}_low_voltage operations and set the low-voltage
capability flag for the associated 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 | 67 +++++++++++++++++++++++++++++++++++-
 3 files changed, 68 insertions(+), 2 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..38be7cbea4ca 100644
--- a/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
+++ b/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
@@ -1739,10 +1739,20 @@ static const u16 pinmux_data[] = {
 #define PIN_NUMBER(r, c) (((r) - 'A') * 31 + (c) + 200)
 #define PIN_A_NUMBER(r, c) PIN_NUMBER(ROW_GROUP_A(r), c)
 
+#define PIN_LOW_VOLTAGE(bank, _pin, _name, sfx)		\
+	[RCAR_GP_PIN(bank, _pin)].configs = SH_PFC_PIN_CFG_LOW_VOLTAGE
+
 static const struct sh_pfc_pin pinmux_pins[] = {
 	PINMUX_GPIO_GP_ALL(),
 
-	/* Pins not associated with a GPIO port */
+	/*
+	 * All pins assigned to GPIO bank 3 can be used for SD interfaces
+	 * in which case they support low voltage (1.8V) signalling.
+	 */
+	PORT_GP_32(3, PIN_LOW_VOLTAGE, unused),
+
+	/* Pins not associated with a GPIO port, placed after all the GPIOs */
+	[RCAR_GP_PIN(5, 31) + 1]  	SH_PFC_PIN_NAMED(ROW_GROUP_A('F'), 15, AF15),
 	SH_PFC_PIN_NAMED(ROW_GROUP_A('G'), 15, AG15),
 	SH_PFC_PIN_NAMED(ROW_GROUP_A('H'), 15, AH15),
@@ -4595,6 +4605,55 @@ static const char * const vin3_groups[] = {
 	"vin3_clk",
 };
 
+static bool sdhi_get_low_voltage(struct sh_pfc *pfc, unsigned int pin)
+{
+	void __iomem *mapped_reg;
+	u32 data, mask;
+
+	if (WARN(pin < RCAR_GP_PIN(3, 0) || pin > RCAR_GP_PIN(3, 31),
+		 "sdhi_get_low_voltage: invalid pin %#x", pin))
+		return 0;
+
+	/* Map IOCTRL6 */
+	mapped_reg = sh_pfc_phys_to_virt(pfc, 0xe606008c);
+
+	/* Bits in IOCTRL6 are numbered in opposite order to pins */
+	mask = 0x80000000 >> (pin & 0x1f);
+
+	data = sh_pfc_read_raw_reg(mapped_reg, 32);
+
+	return !(data & mask);
+}
+
+static void
+sdhi_set_low_voltage(struct sh_pfc *pfc, unsigned int pin, bool low)
+{
+	void __iomem *mapped_reg;
+	u32 data, mask;
+
+	if (WARN(pin < RCAR_GP_PIN(3, 0) || pin > RCAR_GP_PIN(3, 31),
+		 "sdhi_set_low_voltage: invalid pin %#x", pin))
+		return;
+
+	/* Map IOCTRL6 */
+	mapped_reg = sh_pfc_phys_to_virt(pfc, 0xe606008c);
+
+	/* Bits in IOCTRL6 are numbered in opposite order to pins */
+	mask = 0x80000000 >> (pin & 0x1f);
+
+	data = sh_pfc_read_raw_reg(mapped_reg, 32);
+
+	if (low)
+		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),
@@ -5586,8 +5645,14 @@ static const struct pinmux_cfg_reg pinmux_config_regs[] = {
 	{ },
 };
 
+static const struct sh_pfc_soc_operations pinmux_ops = {
+	.get_low_voltage = sdhi_get_low_voltage,
+	.set_low_voltage = sdhi_set_low_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.1.4

[PATCH v3 3/6] pinctrl: sh-pfc: r8a7790: Implement voltage switching for SDHI

[Ben Hutchings]

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

Implement the {get,set}_low_voltage operations and set the low-voltage
capability flag for the associated 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 | 67 +++++++++++++++++++++++++++++++++++-
 3 files changed, 68 insertions(+), 2 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..38be7cbea4ca 100644
--- a/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
+++ b/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
@@ -1739,10 +1739,20 @@ static const u16 pinmux_data[] = {
 #define PIN_NUMBER(r, c) (((r) - 'A') * 31 + (c) + 200)
 #define PIN_A_NUMBER(r, c) PIN_NUMBER(ROW_GROUP_A(r), c)
 
+#define PIN_LOW_VOLTAGE(bank, _pin, _name, sfx)		\
+	[RCAR_GP_PIN(bank, _pin)].configs = SH_PFC_PIN_CFG_LOW_VOLTAGE
+
 static const struct sh_pfc_pin pinmux_pins[] = {
 	PINMUX_GPIO_GP_ALL(),
 
-	/* Pins not associated with a GPIO port */
+	/*
+	 * All pins assigned to GPIO bank 3 can be used for SD interfaces
+	 * in which case they support low voltage (1.8V) signalling.
+	 */
+	PORT_GP_32(3, PIN_LOW_VOLTAGE, unused),
+
+	/* Pins not associated with a GPIO port, placed after all the GPIOs */
+	[RCAR_GP_PIN(5, 31) + 1] =
 	SH_PFC_PIN_NAMED(ROW_GROUP_A('F'), 15, AF15),
 	SH_PFC_PIN_NAMED(ROW_GROUP_A('G'), 15, AG15),
 	SH_PFC_PIN_NAMED(ROW_GROUP_A('H'), 15, AH15),
@@ -4595,6 +4605,55 @@ static const char * const vin3_groups[] = {
 	"vin3_clk",
 };
 
+static bool sdhi_get_low_voltage(struct sh_pfc *pfc, unsigned int pin)
+{
+	void __iomem *mapped_reg;
+	u32 data, mask;
+
+	if (WARN(pin < RCAR_GP_PIN(3, 0) || pin > RCAR_GP_PIN(3, 31),
+		 "sdhi_get_low_voltage: invalid pin %#x", pin))
+		return 0;
+
+	/* Map IOCTRL6 */
+	mapped_reg = sh_pfc_phys_to_virt(pfc, 0xe606008c);
+
+	/* Bits in IOCTRL6 are numbered in opposite order to pins */
+	mask = 0x80000000 >> (pin & 0x1f);
+
+	data = sh_pfc_read_raw_reg(mapped_reg, 32);
+
+	return !(data & mask);
+}
+
+static void
+sdhi_set_low_voltage(struct sh_pfc *pfc, unsigned int pin, bool low)
+{
+	void __iomem *mapped_reg;
+	u32 data, mask;
+
+	if (WARN(pin < RCAR_GP_PIN(3, 0) || pin > RCAR_GP_PIN(3, 31),
+		 "sdhi_set_low_voltage: invalid pin %#x", pin))
+		return;
+
+	/* Map IOCTRL6 */
+	mapped_reg = sh_pfc_phys_to_virt(pfc, 0xe606008c);
+
+	/* Bits in IOCTRL6 are numbered in opposite order to pins */
+	mask = 0x80000000 >> (pin & 0x1f);
+
+	data = sh_pfc_read_raw_reg(mapped_reg, 32);
+
+	if (low)
+		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),
@@ -5586,8 +5645,14 @@ static const struct pinmux_cfg_reg pinmux_config_regs[] = {
 	{ },
 };
 
+static const struct sh_pfc_soc_operations pinmux_ops = {
+	.get_low_voltage = sdhi_get_low_voltage,
+	.set_low_voltage = sdhi_set_low_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.1.4

[PATCH v3 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 | 60 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 354f4f335ed5..2e65b855a7f7 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,49 @@ 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;
+
+	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;
@@ -228,6 +276,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 +295,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;
-- 
2.1.4

[PATCH v3 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 | 60 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 354f4f335ed5..2e65b855a7f7 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,49 @@ 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;
+
+	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;
@@ -228,6 +276,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 +295,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;
-- 
2.1.4

[PATCH v3 5/6] ARM: shmobile: lager: Set clock rates for SDHI

[Ben Hutchings]

Set the input clocks to the highest supported speeds.

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
 arch/arm/boot/dts/r8a7790-lager.dts | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
index aaa4f258e279..5f68e53c58ae 100644
--- a/arch/arm/boot/dts/r8a7790-lager.dts
+++ b/arch/arm/boot/dts/r8a7790-lager.dts
@@ -488,6 +488,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 +501,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>;
-- 
2.1.4

[PATCH v3 5/6] ARM: shmobile: lager: Set clock rates for SDHI

[Ben Hutchings]

Set the input clocks to the highest supported speeds.

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
 arch/arm/boot/dts/r8a7790-lager.dts | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
index aaa4f258e279..5f68e53c58ae 100644
--- a/arch/arm/boot/dts/r8a7790-lager.dts
+++ b/arch/arm/boot/dts/r8a7790-lager.dts
@@ -488,6 +488,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 +501,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>;
-- 
2.1.4

[PATCH v3 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>
---
 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 5f68e53c58ae..c7db213054f6 100644
--- a/arch/arm/boot/dts/r8a7790-lager.dts
+++ b/arch/arm/boot/dts/r8a7790-lager.dts
@@ -312,11 +312,25 @@
 	sdhi0_pins: sd0 {
 		renesas,groups = "sdhi0_data4", "sdhi0_ctrl";
 		renesas,function = "sdhi0";
+		power-source = <0>;
+	};
+
+	sdhi0_pins_1v8: sd0_1v8 {
+		renesas,groups = "sdhi0_data4", "sdhi0_ctrl";
+		renesas,function = "sdhi0";
+		power-source = <1>;
 	};
 
 	sdhi2_pins: sd2 {
 		renesas,groups = "sdhi2_data4", "sdhi2_ctrl";
 		renesas,function = "sdhi2";
+		power-source = <0>;
+	};
+
+	sdhi2_pins_1v8: sd2_1v8 {
+		renesas,groups = "sdhi2_data4", "sdhi2_ctrl";
+		renesas,function = "sdhi2";
+		power-source = <1>;
 	};
 
 	mmc1_pins: mmc1 {
@@ -486,7 +500,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>;
@@ -494,12 +509,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>;
@@ -507,6 +524,7 @@
 	vmmc-supply = <&vcc_sdhi2>;
 	vqmmc-supply = <&vccq_sdhi2>;
 	cd-gpios = <&gpio3 22 GPIO_ACTIVE_LOW>;
+	sd-uhs-sdr50;
 	status = "okay";
 };
 
-- 
2.1.4

[PATCH v3 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>
---
 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 5f68e53c58ae..c7db213054f6 100644
--- a/arch/arm/boot/dts/r8a7790-lager.dts
+++ b/arch/arm/boot/dts/r8a7790-lager.dts
@@ -312,11 +312,25 @@
 	sdhi0_pins: sd0 {
 		renesas,groups = "sdhi0_data4", "sdhi0_ctrl";
 		renesas,function = "sdhi0";
+		power-source = <0>;
+	};
+
+	sdhi0_pins_1v8: sd0_1v8 {
+		renesas,groups = "sdhi0_data4", "sdhi0_ctrl";
+		renesas,function = "sdhi0";
+		power-source = <1>;
 	};
 
 	sdhi2_pins: sd2 {
 		renesas,groups = "sdhi2_data4", "sdhi2_ctrl";
 		renesas,function = "sdhi2";
+		power-source = <0>;
+	};
+
+	sdhi2_pins_1v8: sd2_1v8 {
+		renesas,groups = "sdhi2_data4", "sdhi2_ctrl";
+		renesas,function = "sdhi2";
+		power-source = <1>;
 	};
 
 	mmc1_pins: mmc1 {
@@ -486,7 +500,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>;
@@ -494,12 +509,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>;
@@ -507,6 +524,7 @@
 	vmmc-supply = <&vcc_sdhi2>;
 	vqmmc-supply = <&vccq_sdhi2>;
 	cd-gpios = <&gpio3 22 GPIO_ACTIVE_LOW>;
+	sd-uhs-sdr50;
 	status = "okay";
 };
 
-- 
2.1.4

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

[Ben Hutchings]

I accidentally used an older shortlog and diffstat in that cover mail;
here are the correct ones.

Ben.

Ben Hutchings (5):
  mmc: tmio: Add UHS-I mode support
  pinctrl: sh-pfc: Implement pinconf power-source param for voltage
    switching
  pinctrl: sh-pfc: r8a7790: Implement voltage switching for SDHI
  mmc: sh_mobile_sdhi: Add UHS-I mode support
  ARM: shmobile: lager: Enable UHS-I SDR-50

Ian Molton (1):
  ARM: shmobile: lager: Set clock rates for SDHI

 .../bindings/pinctrl/renesas,pfc-pinctrl.txt       |  4 +-
 arch/arm/boot/dts/r8a7790-lager.dts                | 28 ++++++++-
 drivers/mmc/host/sh_mobile_sdhi.c                  | 60 +++++++++++++++++++
 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               | 67 +++++++++++++++++++++-
 drivers/pinctrl/sh-pfc/pinctrl.c                   | 34 ++++++++++-
 drivers/pinctrl/sh-pfc/sh_pfc.h                    |  6 ++
 10 files changed, 229 insertions(+), 7 deletions(-)

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

[Ben Hutchings]

I accidentally used an older shortlog and diffstat in that cover mail;
here are the correct ones.

Ben.

Ben Hutchings (5):
  mmc: tmio: Add UHS-I mode support
  pinctrl: sh-pfc: Implement pinconf power-source param for voltage
    switching
  pinctrl: sh-pfc: r8a7790: Implement voltage switching for SDHI
  mmc: sh_mobile_sdhi: Add UHS-I mode support
  ARM: shmobile: lager: Enable UHS-I SDR-50

Ian Molton (1):
  ARM: shmobile: lager: Set clock rates for SDHI

 .../bindings/pinctrl/renesas,pfc-pinctrl.txt       |  4 +-
 arch/arm/boot/dts/r8a7790-lager.dts                | 28 ++++++++-
 drivers/mmc/host/sh_mobile_sdhi.c                  | 60 +++++++++++++++++++
 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               | 67 +++++++++++++++++++++-
 drivers/pinctrl/sh-pfc/pinctrl.c                   | 34 ++++++++++-
 drivers/pinctrl/sh-pfc/sh_pfc.h                    |  6 ++
 10 files changed, 229 insertions(+), 7 deletions(-)

Re: [PATCH v3 5/6] ARM: shmobile: lager: Set clock rates for SDHI

[Kuninori Morimoto]

Hi Ben 
Cc Laurent, Geert, Magnus

> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
> index aaa4f258e279..5f68e53c58ae 100644
> --- a/arch/arm/boot/dts/r8a7790-lager.dts
> +++ b/arch/arm/boot/dts/r8a7790-lager.dts
> @@ -488,6 +488,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 +501,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>;

Thank you for your patch, but I still wandering about this.
Please correct me if I'm misunderstanding.

Above seetings is for SDHI IP, and it can divide it ?
The image is

	[CPG] -> 156 MHz -> [SDHI] -> 1/x -> [CARD]
	         ~~~~~~~

If so, why we can't use max-frequency ?
We can calculate/set SDHI IP clocks via 
max-frequency / clk_round_rate() / clk_set_rate()
since we know SDHI's divider capability.

SH-MMC is using this style. and I think it is flexible for every speed.
Please check sh_mmcif_clock_control(), sh_mmcif_clk_setup()
on ${LINUX}/drivers/mmc/host/sh_mmcif.c

Best regards
---
Kuninori Morimoto

Re: [PATCH v3 2/6] pinctrl: sh-pfc: Implement pinconf power-source param for voltage switching

[Geert Uytterhoeven]

On Fri, Jun 26, 2015 at 5:23 PM, Ben Hutchings
<ben.hutchings@codethink.co.uk> wrote:
> The pfc in the R8A7790 (and probably others in the R-Car gen 2 family)
> supports switching SDHI signals between 3.3V and 1.8V voltage, and the
> SD driver should do that when selecting a higher-speed mode.
>
> Add a flag for pins that support low voltage mode and SoC operations to
> get and set it.  Implement the pinconf power-source parameter using these
> operations.
>
> Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> ---
>  .../bindings/pinctrl/renesas,pfc-pinctrl.txt       |  4 ++-
>  drivers/pinctrl/sh-pfc/pinctrl.c                   | 34 ++++++++++++++++++++--
>  drivers/pinctrl/sh-pfc/sh_pfc.h                    |  6 ++++
>  3 files changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
> index bfe72ec055e3..93651742f1c1 100644
> --- a/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
> @@ -69,7 +69,9 @@ Pin Configuration Node Properties:
>
>  The pin configuration parameters use the generic pinconf bindings defined in
>  pinctrl-bindings.txt in this directory. The supported parameters are
> -bias-disable, bias-pull-up and bias-pull-down.
> +bias-disable, bias-pull-up, bias-pull-down and power-source. For pins that
> +can operate with either 3.3V or 1.8V signals, the power-source value should
> +be 0 for 3.3V or 1 for 1.8V.

> @@ -121,6 +125,8 @@ struct sh_pfc_soc_operations {
>         unsigned int (*get_bias)(struct sh_pfc *pfc, unsigned int pin);
>         void (*set_bias)(struct sh_pfc *pfc, unsigned int pin,
>                          unsigned int bias);
> +       bool (*get_low_voltage)(struct sh_pfc *pfc, unsigned int pin);
> +       void (*set_low_voltage)(struct sh_pfc *pfc, unsigned int pin, bool low);
>  };

Given the sh-pfc driver is used for a whole range of SoCs, I think a boolean
value is a bit too limiting.

E.g. In addition to 1.8V and 3.3 V, R-Car M2 has pins supporting three
voltage levels (1.35V, 1.5V, and 18.V).
R-Mobile APE6 supports 1.2V, 1.8V, and 3.3V, but max. only 2 out of 3 for
a single pin.

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 v3 2/6] pinctrl: sh-pfc: Implement pinconf power-source param for voltage switching

[Geert Uytterhoeven]

On Fri, Jun 26, 2015 at 5:23 PM, Ben Hutchings
<ben.hutchings@codethink.co.uk> wrote:
> The pfc in the R8A7790 (and probably others in the R-Car gen 2 family)
> supports switching SDHI signals between 3.3V and 1.8V voltage, and the
> SD driver should do that when selecting a higher-speed mode.
>
> Add a flag for pins that support low voltage mode and SoC operations to
> get and set it.  Implement the pinconf power-source parameter using these
> operations.
>
> Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> ---
>  .../bindings/pinctrl/renesas,pfc-pinctrl.txt       |  4 ++-
>  drivers/pinctrl/sh-pfc/pinctrl.c                   | 34 ++++++++++++++++++++--
>  drivers/pinctrl/sh-pfc/sh_pfc.h                    |  6 ++++
>  3 files changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
> index bfe72ec055e3..93651742f1c1 100644
> --- a/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
> @@ -69,7 +69,9 @@ Pin Configuration Node Properties:
>
>  The pin configuration parameters use the generic pinconf bindings defined in
>  pinctrl-bindings.txt in this directory. The supported parameters are
> -bias-disable, bias-pull-up and bias-pull-down.
> +bias-disable, bias-pull-up, bias-pull-down and power-source. For pins that
> +can operate with either 3.3V or 1.8V signals, the power-source value should
> +be 0 for 3.3V or 1 for 1.8V.

> @@ -121,6 +125,8 @@ struct sh_pfc_soc_operations {
>         unsigned int (*get_bias)(struct sh_pfc *pfc, unsigned int pin);
>         void (*set_bias)(struct sh_pfc *pfc, unsigned int pin,
>                          unsigned int bias);
> +       bool (*get_low_voltage)(struct sh_pfc *pfc, unsigned int pin);
> +       void (*set_low_voltage)(struct sh_pfc *pfc, unsigned int pin, bool low);
>  };

Given the sh-pfc driver is used for a whole range of SoCs, I think a boolean
value is a bit too limiting.

E.g. In addition to 1.8V and 3.3 V, R-Car M2 has pins supporting three
voltage levels (1.35V, 1.5V, and 18.V).
R-Mobile APE6 supports 1.2V, 1.8V, and 3.3V, but max. only 2 out of 3 for
a single pin.

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 v3 2/6] pinctrl: sh-pfc: Implement pinconf power-source param for voltage switching

[Laurent Pinchart]

Hi Ben,

Thank you for the patch.

On Friday 26 June 2015 16:23:24 Ben Hutchings wrote:
> The pfc in the R8A7790 (and probably others in the R-Car gen 2 family)
> supports switching SDHI signals between 3.3V and 1.8V voltage, and the
> SD driver should do that when selecting a higher-speed mode.
> 
> Add a flag for pins that support low voltage mode and SoC operations to
> get and set it.  Implement the pinconf power-source parameter using these
> operations.

As Geert has already pointed out, we should use an integer value instead of a 
boolean for voltages. How about expressing them in microvolts as done by the 
regulators API ?

Apart from that the patch looks quite good to me. Please see below for a 
couple of other comments.

> Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> ---
>  .../bindings/pinctrl/renesas,pfc-pinctrl.txt       |  4 ++-
>  drivers/pinctrl/sh-pfc/pinctrl.c                   | 34 +++++++++++++++++--
>  drivers/pinctrl/sh-pfc/sh_pfc.h                    |  6 ++++
>  3 files changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git
> a/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
> b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt index
> bfe72ec055e3..93651742f1c1 100644
> --- a/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
> @@ -69,7 +69,9 @@ Pin Configuration Node Properties:
> 
>  The pin configuration parameters use the generic pinconf bindings defined
> in pinctrl-bindings.txt in this directory. The supported parameters are
> -bias-disable, bias-pull-up and bias-pull-down.
> +bias-disable, bias-pull-up, bias-pull-down and power-source. For pins that
> +can operate with either 3.3V or 1.8V signals, the power-source value should
> +be 0 for 3.3V or 1 for 1.8V.
> 
> 
>  GPIO
> diff --git a/drivers/pinctrl/sh-pfc/pinctrl.c
> b/drivers/pinctrl/sh-pfc/pinctrl.c index 072e7c62cab7..fa4066d4bc61 100644
> --- a/drivers/pinctrl/sh-pfc/pinctrl.c
> +++ b/drivers/pinctrl/sh-pfc/pinctrl.c
> @@ -465,6 +465,9 @@ static bool sh_pfc_pinconf_validate(struct sh_pfc *pfc,
> unsigned int _pin, case PIN_CONFIG_BIAS_PULL_DOWN:
>  		return pin->configs & SH_PFC_PIN_CFG_PULL_DOWN;
> 
> +	case PIN_CONFIG_POWER_SOURCE:
> +		return pin->configs & SH_PFC_PIN_CFG_LOW_VOLTAGE;
> +
>  	default:
>  		return false;
>  	}
> @@ -477,7 +480,6 @@ static int sh_pfc_pinconf_get(struct pinctrl_dev
> *pctldev, unsigned _pin, struct sh_pfc *pfc = pmx->pfc;
>  	enum pin_config_param param = pinconf_to_config_param(*config);
>  	unsigned long flags;
> -	unsigned int bias;
> 
>  	if (!sh_pfc_pinconf_validate(pfc, _pin, param))
>  		return -ENOTSUPP;
> @@ -485,7 +487,9 @@ static int sh_pfc_pinconf_get(struct pinctrl_dev
> *pctldev, unsigned _pin, switch (param) {
>  	case PIN_CONFIG_BIAS_DISABLE:
>  	case PIN_CONFIG_BIAS_PULL_UP:
> -	case PIN_CONFIG_BIAS_PULL_DOWN:
> +	case PIN_CONFIG_BIAS_PULL_DOWN: {
> +		unsigned int bias;
> +
>  		if (!pfc->info->ops || !pfc->info->ops->get_bias)
>  			return -ENOTSUPP;
> 
> @@ -498,6 +502,16 @@ static int sh_pfc_pinconf_get(struct pinctrl_dev
> *pctldev, unsigned _pin,
> 
>  		*config = 0;
>  		break;
> +	}
> +
> +	case PIN_CONFIG_POWER_SOURCE:
> +		if (!pfc->info->ops || !pfc->info->ops->get_low_voltage)
> +			return -ENOTSUPP;
> +
> +		spin_lock_irqsave(&pfc->lock, flags);
> +		*config = pfc->info->ops->get_low_voltage(pfc, _pin);
> +		spin_unlock_irqrestore(&pfc->lock, flags);
> +		break;
> 
>  	default:
>  		return -ENOTSUPP;
> @@ -534,6 +548,22 @@ static int sh_pfc_pinconf_set(struct pinctrl_dev
> *pctldev, unsigned _pin,
> 
>  			break;
> 
> +		case PIN_CONFIG_POWER_SOURCE: {
> +			unsigned int arg > +				pinconf_to_config_argument(configs[i]);
> +
> +			if (!pfc->info->ops || !pfc->info->ops->set_low_voltage)
> +				return -ENOTSUPP;
> +			if (arg > 1)
> +				return -ENOTSUPP;
> +
> +			spin_lock_irqsave(&pfc->lock, flags);
> +			pfc->info->ops->set_low_voltage(pfc, _pin, arg);
> +			spin_unlock_irqrestore(&pfc->lock, flags);
> +
> +			break;
> +		}
> +
>  		default:
>  			return -ENOTSUPP;
>  		}
> diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h
> b/drivers/pinctrl/sh-pfc/sh_pfc.h index c7508d5f6886..b95d60bf2f1b 100644
> --- a/drivers/pinctrl/sh-pfc/sh_pfc.h
> +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
> @@ -13,6 +13,7 @@
> 
>  #include <linux/bug.h>
>  #include <linux/stringify.h>
> +#include <linux/pinctrl/pinconf-generic.h>

Could you please keep the headers alphabetically sorted ?

> 
>  enum {
>  	PINMUX_TYPE_NONE,
> @@ -26,8 +27,11 @@ enum {
>  #define SH_PFC_PIN_CFG_OUTPUT		(1 << 1)
>  #define SH_PFC_PIN_CFG_PULL_UP		(1 << 2)
>  #define SH_PFC_PIN_CFG_PULL_DOWN	(1 << 3)
> +#define SH_PFC_PIN_CFG_LOW_VOLTAGE	(1 << 4)

As we should move away from using a bool to represent the voltage, I would 
name this SH_PFC_PIN_CFG_IO_VOLTAGE, or possibly SH_PFC_PIN_CFG_POWER_SOURCE.

>  #define SH_PFC_PIN_CFG_NO_GPIO		(1 << 31)
> 
> +struct sh_pfc;
> +

I don't think this is needed, struct sh_pfc is already forward-declared right 
above struct sh_pfc_soc_operations.

>  struct sh_pfc_pin {
>  	u16 pin;
>  	u16 enum_id;
> @@ -121,6 +125,8 @@ struct sh_pfc_soc_operations {
>  	unsigned int (*get_bias)(struct sh_pfc *pfc, unsigned int pin);
>  	void (*set_bias)(struct sh_pfc *pfc, unsigned int pin,
>  			 unsigned int bias);
> +	bool (*get_low_voltage)(struct sh_pfc *pfc, unsigned int pin);
> +	void (*set_low_voltage)(struct sh_pfc *pfc, unsigned int pin, bool low);

[gs]et_io_voltage() or [gs]et_power_source() ?

>  };
> 
>  struct sh_pfc_soc_info {

-- 
Regards,

Laurent Pinchart
-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 2/6] pinctrl: sh-pfc: Implement pinconf power-source param for voltage switching

[Laurent Pinchart]

Hi Ben,

Thank you for the patch.

On Friday 26 June 2015 16:23:24 Ben Hutchings wrote:
> The pfc in the R8A7790 (and probably others in the R-Car gen 2 family)
> supports switching SDHI signals between 3.3V and 1.8V voltage, and the
> SD driver should do that when selecting a higher-speed mode.
> 
> Add a flag for pins that support low voltage mode and SoC operations to
> get and set it.  Implement the pinconf power-source parameter using these
> operations.

As Geert has already pointed out, we should use an integer value instead of a 
boolean for voltages. How about expressing them in microvolts as done by the 
regulators API ?

Apart from that the patch looks quite good to me. Please see below for a 
couple of other comments.

> Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> ---
>  .../bindings/pinctrl/renesas,pfc-pinctrl.txt       |  4 ++-
>  drivers/pinctrl/sh-pfc/pinctrl.c                   | 34 +++++++++++++++++--
>  drivers/pinctrl/sh-pfc/sh_pfc.h                    |  6 ++++
>  3 files changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git
> a/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
> b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt index
> bfe72ec055e3..93651742f1c1 100644
> --- a/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
> @@ -69,7 +69,9 @@ Pin Configuration Node Properties:
> 
>  The pin configuration parameters use the generic pinconf bindings defined
> in pinctrl-bindings.txt in this directory. The supported parameters are
> -bias-disable, bias-pull-up and bias-pull-down.
> +bias-disable, bias-pull-up, bias-pull-down and power-source. For pins that
> +can operate with either 3.3V or 1.8V signals, the power-source value should
> +be 0 for 3.3V or 1 for 1.8V.
> 
> 
>  GPIO
> diff --git a/drivers/pinctrl/sh-pfc/pinctrl.c
> b/drivers/pinctrl/sh-pfc/pinctrl.c index 072e7c62cab7..fa4066d4bc61 100644
> --- a/drivers/pinctrl/sh-pfc/pinctrl.c
> +++ b/drivers/pinctrl/sh-pfc/pinctrl.c
> @@ -465,6 +465,9 @@ static bool sh_pfc_pinconf_validate(struct sh_pfc *pfc,
> unsigned int _pin, case PIN_CONFIG_BIAS_PULL_DOWN:
>  		return pin->configs & SH_PFC_PIN_CFG_PULL_DOWN;
> 
> +	case PIN_CONFIG_POWER_SOURCE:
> +		return pin->configs & SH_PFC_PIN_CFG_LOW_VOLTAGE;
> +
>  	default:
>  		return false;
>  	}
> @@ -477,7 +480,6 @@ static int sh_pfc_pinconf_get(struct pinctrl_dev
> *pctldev, unsigned _pin, struct sh_pfc *pfc = pmx->pfc;
>  	enum pin_config_param param = pinconf_to_config_param(*config);
>  	unsigned long flags;
> -	unsigned int bias;
> 
>  	if (!sh_pfc_pinconf_validate(pfc, _pin, param))
>  		return -ENOTSUPP;
> @@ -485,7 +487,9 @@ static int sh_pfc_pinconf_get(struct pinctrl_dev
> *pctldev, unsigned _pin, switch (param) {
>  	case PIN_CONFIG_BIAS_DISABLE:
>  	case PIN_CONFIG_BIAS_PULL_UP:
> -	case PIN_CONFIG_BIAS_PULL_DOWN:
> +	case PIN_CONFIG_BIAS_PULL_DOWN: {
> +		unsigned int bias;
> +
>  		if (!pfc->info->ops || !pfc->info->ops->get_bias)
>  			return -ENOTSUPP;
> 
> @@ -498,6 +502,16 @@ static int sh_pfc_pinconf_get(struct pinctrl_dev
> *pctldev, unsigned _pin,
> 
>  		*config = 0;
>  		break;
> +	}
> +
> +	case PIN_CONFIG_POWER_SOURCE:
> +		if (!pfc->info->ops || !pfc->info->ops->get_low_voltage)
> +			return -ENOTSUPP;
> +
> +		spin_lock_irqsave(&pfc->lock, flags);
> +		*config = pfc->info->ops->get_low_voltage(pfc, _pin);
> +		spin_unlock_irqrestore(&pfc->lock, flags);
> +		break;
> 
>  	default:
>  		return -ENOTSUPP;
> @@ -534,6 +548,22 @@ static int sh_pfc_pinconf_set(struct pinctrl_dev
> *pctldev, unsigned _pin,
> 
>  			break;
> 
> +		case PIN_CONFIG_POWER_SOURCE: {
> +			unsigned int arg =
> +				pinconf_to_config_argument(configs[i]);
> +
> +			if (!pfc->info->ops || !pfc->info->ops->set_low_voltage)
> +				return -ENOTSUPP;
> +			if (arg > 1)
> +				return -ENOTSUPP;
> +
> +			spin_lock_irqsave(&pfc->lock, flags);
> +			pfc->info->ops->set_low_voltage(pfc, _pin, arg);
> +			spin_unlock_irqrestore(&pfc->lock, flags);
> +
> +			break;
> +		}
> +
>  		default:
>  			return -ENOTSUPP;
>  		}
> diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h
> b/drivers/pinctrl/sh-pfc/sh_pfc.h index c7508d5f6886..b95d60bf2f1b 100644
> --- a/drivers/pinctrl/sh-pfc/sh_pfc.h
> +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
> @@ -13,6 +13,7 @@
> 
>  #include <linux/bug.h>
>  #include <linux/stringify.h>
> +#include <linux/pinctrl/pinconf-generic.h>

Could you please keep the headers alphabetically sorted ?

> 
>  enum {
>  	PINMUX_TYPE_NONE,
> @@ -26,8 +27,11 @@ enum {
>  #define SH_PFC_PIN_CFG_OUTPUT		(1 << 1)
>  #define SH_PFC_PIN_CFG_PULL_UP		(1 << 2)
>  #define SH_PFC_PIN_CFG_PULL_DOWN	(1 << 3)
> +#define SH_PFC_PIN_CFG_LOW_VOLTAGE	(1 << 4)

As we should move away from using a bool to represent the voltage, I would 
name this SH_PFC_PIN_CFG_IO_VOLTAGE, or possibly SH_PFC_PIN_CFG_POWER_SOURCE.

>  #define SH_PFC_PIN_CFG_NO_GPIO		(1 << 31)
> 
> +struct sh_pfc;
> +

I don't think this is needed, struct sh_pfc is already forward-declared right 
above struct sh_pfc_soc_operations.

>  struct sh_pfc_pin {
>  	u16 pin;
>  	u16 enum_id;
> @@ -121,6 +125,8 @@ struct sh_pfc_soc_operations {
>  	unsigned int (*get_bias)(struct sh_pfc *pfc, unsigned int pin);
>  	void (*set_bias)(struct sh_pfc *pfc, unsigned int pin,
>  			 unsigned int bias);
> +	bool (*get_low_voltage)(struct sh_pfc *pfc, unsigned int pin);
> +	void (*set_low_voltage)(struct sh_pfc *pfc, unsigned int pin, bool low);

[gs]et_io_voltage() or [gs]et_power_source() ?

>  };
> 
>  struct sh_pfc_soc_info {

-- 
Regards,

Laurent Pinchart
-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 3/6] pinctrl: sh-pfc: r8a7790: Implement voltage switching for SDHI

[Laurent Pinchart]

Hi Ben,

Thank you for the patch.

On Friday 26 June 2015 16:23:30 Ben Hutchings wrote:
> All the SHDIs can operate with either 3.3V or 1.8V signals, depending
> on negotiation with the card.
> 
> Implement the {get,set}_low_voltage operations and set the low-voltage
> capability flag for the associated 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 | 67 ++++++++++++++++++++++++++++++++-
>  3 files changed, 68 insertions(+), 2
> 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..38be7cbea4ca
> 100644
> --- a/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
> +++ b/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
> @@ -1739,10 +1739,20 @@ static const u16 pinmux_data[] = {
>  #define PIN_NUMBER(r, c) (((r) - 'A') * 31 + (c) + 200)
>  #define PIN_A_NUMBER(r, c) PIN_NUMBER(ROW_GROUP_A(r), c)
> 
> +#define PIN_LOW_VOLTAGE(bank, _pin, _name, sfx)		\
> +	[RCAR_GP_PIN(bank, _pin)].configs = SH_PFC_PIN_CFG_LOW_VOLTAGE
> +
>  static const struct sh_pfc_pin pinmux_pins[] = {
>  	PINMUX_GPIO_GP_ALL(),
> 
> -	/* Pins not associated with a GPIO port */
> +	/*
> +	 * All pins assigned to GPIO bank 3 can be used for SD interfaces
> +	 * in which case they support low voltage (1.8V) signalling.
> +	 */
> +	PORT_GP_32(3, PIN_LOW_VOLTAGE, unused),

Ouch. I didn't know gcc would even support initializing fields of array member 
structures separately from full initialization of the array member structure. 
Is it standard C ? Is it supported by LLVM ? I would prefer replacing 
PINMUX_GPIO_GP_ALL instead with a version that initializes the config field 
appropriately.

Another idea I've been toying with is it replace the config field by an 
operation that returns the config value for a given pin. It would increase 
execution time slightly, but would likely save memory as the logic is often 
easy to express in C code and doesn't require a per-pin array.

> +	/* Pins not associated with a GPIO port, placed after all the GPIOs */
> +	[RCAR_GP_PIN(5, 31) + 1] >  	SH_PFC_PIN_NAMED(ROW_GROUP_A('F'), 15, AF15),
>  	SH_PFC_PIN_NAMED(ROW_GROUP_A('G'), 15, AG15),
>  	SH_PFC_PIN_NAMED(ROW_GROUP_A('H'), 15, AH15),
> @@ -4595,6 +4605,55 @@ static const char * const vin3_groups[] = {
>  	"vin3_clk",
>  };
> 
> +static bool sdhi_get_low_voltage(struct sh_pfc *pfc, unsigned int pin)
> +{
> +	void __iomem *mapped_reg;
> +	u32 data, mask;
> +
> +	if (WARN(pin < RCAR_GP_PIN(3, 0) || pin > RCAR_GP_PIN(3, 31),
> +		 "sdhi_get_low_voltage: invalid pin %#x", pin))
> +		return 0;

If the rest of the driver behaves correctly this should never happen, right ?

> +	/* Map IOCTRL6 */
> +	mapped_reg = sh_pfc_phys_to_virt(pfc, 0xe606008c);
> +
> +	/* Bits in IOCTRL6 are numbered in opposite order to pins */
> +	mask = 0x80000000 >> (pin & 0x1f);
> +
> +	data = sh_pfc_read_raw_reg(mapped_reg, 32);

Given that we know the register width here I would replace this with a direct 
call to ioread32(). Same for the read and write calls below.

> +
> +	return !(data & mask);
> +}
> +
> +static void
> +sdhi_set_low_voltage(struct sh_pfc *pfc, unsigned int pin, bool low)
> +{
> +	void __iomem *mapped_reg;
> +	u32 data, mask;
> +
> +	if (WARN(pin < RCAR_GP_PIN(3, 0) || pin > RCAR_GP_PIN(3, 31),
> +		 "sdhi_set_low_voltage: invalid pin %#x", pin))
> +		return;
> +
> +	/* Map IOCTRL6 */
> +	mapped_reg = sh_pfc_phys_to_virt(pfc, 0xe606008c);
> +
> +	/* Bits in IOCTRL6 are numbered in opposite order to pins */
> +	mask = 0x80000000 >> (pin & 0x1f);
> +
> +	data = sh_pfc_read_raw_reg(mapped_reg, 32);
> +
> +	if (low)
> +		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),
> @@ -5586,8 +5645,14 @@ static const struct pinmux_cfg_reg
> pinmux_config_regs[] = { { },
>  };
> 
> +static const struct sh_pfc_soc_operations pinmux_ops = {
> +	.get_low_voltage = sdhi_get_low_voltage,
> +	.set_low_voltage = sdhi_set_low_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 },

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 3/6] pinctrl: sh-pfc: r8a7790: Implement voltage switching for SDHI

[Laurent Pinchart]

Hi Ben,

Thank you for the patch.

On Friday 26 June 2015 16:23:30 Ben Hutchings wrote:
> All the SHDIs can operate with either 3.3V or 1.8V signals, depending
> on negotiation with the card.
> 
> Implement the {get,set}_low_voltage operations and set the low-voltage
> capability flag for the associated 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 | 67 ++++++++++++++++++++++++++++++++-
>  3 files changed, 68 insertions(+), 2
> 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..38be7cbea4ca
> 100644
> --- a/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
> +++ b/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
> @@ -1739,10 +1739,20 @@ static const u16 pinmux_data[] = {
>  #define PIN_NUMBER(r, c) (((r) - 'A') * 31 + (c) + 200)
>  #define PIN_A_NUMBER(r, c) PIN_NUMBER(ROW_GROUP_A(r), c)
> 
> +#define PIN_LOW_VOLTAGE(bank, _pin, _name, sfx)		\
> +	[RCAR_GP_PIN(bank, _pin)].configs = SH_PFC_PIN_CFG_LOW_VOLTAGE
> +
>  static const struct sh_pfc_pin pinmux_pins[] = {
>  	PINMUX_GPIO_GP_ALL(),
> 
> -	/* Pins not associated with a GPIO port */
> +	/*
> +	 * All pins assigned to GPIO bank 3 can be used for SD interfaces
> +	 * in which case they support low voltage (1.8V) signalling.
> +	 */
> +	PORT_GP_32(3, PIN_LOW_VOLTAGE, unused),

Ouch. I didn't know gcc would even support initializing fields of array member 
structures separately from full initialization of the array member structure. 
Is it standard C ? Is it supported by LLVM ? I would prefer replacing 
PINMUX_GPIO_GP_ALL instead with a version that initializes the config field 
appropriately.

Another idea I've been toying with is it replace the config field by an 
operation that returns the config value for a given pin. It would increase 
execution time slightly, but would likely save memory as the logic is often 
easy to express in C code and doesn't require a per-pin array.

> +	/* Pins not associated with a GPIO port, placed after all the GPIOs */
> +	[RCAR_GP_PIN(5, 31) + 1] =
>  	SH_PFC_PIN_NAMED(ROW_GROUP_A('F'), 15, AF15),
>  	SH_PFC_PIN_NAMED(ROW_GROUP_A('G'), 15, AG15),
>  	SH_PFC_PIN_NAMED(ROW_GROUP_A('H'), 15, AH15),
> @@ -4595,6 +4605,55 @@ static const char * const vin3_groups[] = {
>  	"vin3_clk",
>  };
> 
> +static bool sdhi_get_low_voltage(struct sh_pfc *pfc, unsigned int pin)
> +{
> +	void __iomem *mapped_reg;
> +	u32 data, mask;
> +
> +	if (WARN(pin < RCAR_GP_PIN(3, 0) || pin > RCAR_GP_PIN(3, 31),
> +		 "sdhi_get_low_voltage: invalid pin %#x", pin))
> +		return 0;

If the rest of the driver behaves correctly this should never happen, right ?

> +	/* Map IOCTRL6 */
> +	mapped_reg = sh_pfc_phys_to_virt(pfc, 0xe606008c);
> +
> +	/* Bits in IOCTRL6 are numbered in opposite order to pins */
> +	mask = 0x80000000 >> (pin & 0x1f);
> +
> +	data = sh_pfc_read_raw_reg(mapped_reg, 32);

Given that we know the register width here I would replace this with a direct 
call to ioread32(). Same for the read and write calls below.

> +
> +	return !(data & mask);
> +}
> +
> +static void
> +sdhi_set_low_voltage(struct sh_pfc *pfc, unsigned int pin, bool low)
> +{
> +	void __iomem *mapped_reg;
> +	u32 data, mask;
> +
> +	if (WARN(pin < RCAR_GP_PIN(3, 0) || pin > RCAR_GP_PIN(3, 31),
> +		 "sdhi_set_low_voltage: invalid pin %#x", pin))
> +		return;
> +
> +	/* Map IOCTRL6 */
> +	mapped_reg = sh_pfc_phys_to_virt(pfc, 0xe606008c);
> +
> +	/* Bits in IOCTRL6 are numbered in opposite order to pins */
> +	mask = 0x80000000 >> (pin & 0x1f);
> +
> +	data = sh_pfc_read_raw_reg(mapped_reg, 32);
> +
> +	if (low)
> +		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),
> @@ -5586,8 +5645,14 @@ static const struct pinmux_cfg_reg
> pinmux_config_regs[] = { { },
>  };
> 
> +static const struct sh_pfc_soc_operations pinmux_ops = {
> +	.get_low_voltage = sdhi_get_low_voltage,
> +	.set_low_voltage = sdhi_set_low_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 },

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 2/6] pinctrl: sh-pfc: Implement pinconf power-source param for voltage switching

[Ben Hutchings]

On Mon, 2015-06-29 at 11:32 +0300, Laurent Pinchart wrote:
> Hi Ben,
> 
> Thank you for the patch.
> 
> On Friday 26 June 2015 16:23:24 Ben Hutchings wrote:
> > The pfc in the R8A7790 (and probably others in the R-Car gen 2 family)
> > supports switching SDHI signals between 3.3V and 1.8V voltage, and the
> > SD driver should do that when selecting a higher-speed mode.
> > 
> > Add a flag for pins that support low voltage mode and SoC operations to
> > get and set it.  Implement the pinconf power-source parameter using these
> > operations.
> 
> As Geert has already pointed out, we should use an integer value instead of a 
> boolean for voltages. How about expressing them in microvolts as done by the 
> regulators API ?

I did consider doing that but it seemed like over-engineering.  However,
given what Geert said about the other SoCs it seems worth doing.

However, pinconf arguments are limited to 16 bits so I think I will have
to make the units millivolts not microvolts.

[...]
> > diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h
> > b/drivers/pinctrl/sh-pfc/sh_pfc.h index c7508d5f6886..b95d60bf2f1b 100644
> > --- a/drivers/pinctrl/sh-pfc/sh_pfc.h
> > +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
> > @@ -13,6 +13,7 @@
> > 
> >  #include <linux/bug.h>
> >  #include <linux/stringify.h>
> > +#include <linux/pinctrl/pinconf-generic.h>
> 
> Could you please keep the headers alphabetically sorted ?
> 
> > 
> >  enum {
> >  	PINMUX_TYPE_NONE,
> > @@ -26,8 +27,11 @@ enum {
> >  #define SH_PFC_PIN_CFG_OUTPUT		(1 << 1)
> >  #define SH_PFC_PIN_CFG_PULL_UP		(1 << 2)
> >  #define SH_PFC_PIN_CFG_PULL_DOWN	(1 << 3)
> > +#define SH_PFC_PIN_CFG_LOW_VOLTAGE	(1 << 4)
> 
> As we should move away from using a bool to represent the voltage, I would 
> name this SH_PFC_PIN_CFG_IO_VOLTAGE, or possibly SH_PFC_PIN_CFG_POWER_SOURCE.

Right.

> >  #define SH_PFC_PIN_CFG_NO_GPIO		(1 << 31)
> > 
> > +struct sh_pfc;
> > +
> 
> I don't think this is needed, struct sh_pfc is already forward-declared right 
> above struct sh_pfc_soc_operations.

Right, this is left over from the previous version where I added an
operation to struct sh_pfc_pin.

> >  struct sh_pfc_pin {
> >  	u16 pin;
> >  	u16 enum_id;
> > @@ -121,6 +125,8 @@ struct sh_pfc_soc_operations {
> >  	unsigned int (*get_bias)(struct sh_pfc *pfc, unsigned int pin);
> >  	void (*set_bias)(struct sh_pfc *pfc, unsigned int pin,
> >  			 unsigned int bias);
> > +	bool (*get_low_voltage)(struct sh_pfc *pfc, unsigned int pin);
> > +	void (*set_low_voltage)(struct sh_pfc *pfc, unsigned int pin, bool low);
> 
> [gs]et_io_voltage() or [gs]et_power_source() ?

I prefer the former.

Ben.

Re: [PATCH v3 2/6] pinctrl: sh-pfc: Implement pinconf power-source param for voltage switching

[Ben Hutchings]

On Mon, 2015-06-29 at 11:32 +0300, Laurent Pinchart wrote:
> Hi Ben,
> 
> Thank you for the patch.
> 
> On Friday 26 June 2015 16:23:24 Ben Hutchings wrote:
> > The pfc in the R8A7790 (and probably others in the R-Car gen 2 family)
> > supports switching SDHI signals between 3.3V and 1.8V voltage, and the
> > SD driver should do that when selecting a higher-speed mode.
> > 
> > Add a flag for pins that support low voltage mode and SoC operations to
> > get and set it.  Implement the pinconf power-source parameter using these
> > operations.
> 
> As Geert has already pointed out, we should use an integer value instead of a 
> boolean for voltages. How about expressing them in microvolts as done by the 
> regulators API ?

I did consider doing that but it seemed like over-engineering.  However,
given what Geert said about the other SoCs it seems worth doing.

However, pinconf arguments are limited to 16 bits so I think I will have
to make the units millivolts not microvolts.

[...]
> > diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h
> > b/drivers/pinctrl/sh-pfc/sh_pfc.h index c7508d5f6886..b95d60bf2f1b 100644
> > --- a/drivers/pinctrl/sh-pfc/sh_pfc.h
> > +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
> > @@ -13,6 +13,7 @@
> > 
> >  #include <linux/bug.h>
> >  #include <linux/stringify.h>
> > +#include <linux/pinctrl/pinconf-generic.h>
> 
> Could you please keep the headers alphabetically sorted ?
> 
> > 
> >  enum {
> >  	PINMUX_TYPE_NONE,
> > @@ -26,8 +27,11 @@ enum {
> >  #define SH_PFC_PIN_CFG_OUTPUT		(1 << 1)
> >  #define SH_PFC_PIN_CFG_PULL_UP		(1 << 2)
> >  #define SH_PFC_PIN_CFG_PULL_DOWN	(1 << 3)
> > +#define SH_PFC_PIN_CFG_LOW_VOLTAGE	(1 << 4)
> 
> As we should move away from using a bool to represent the voltage, I would 
> name this SH_PFC_PIN_CFG_IO_VOLTAGE, or possibly SH_PFC_PIN_CFG_POWER_SOURCE.

Right.

> >  #define SH_PFC_PIN_CFG_NO_GPIO		(1 << 31)
> > 
> > +struct sh_pfc;
> > +
> 
> I don't think this is needed, struct sh_pfc is already forward-declared right 
> above struct sh_pfc_soc_operations.

Right, this is left over from the previous version where I added an
operation to struct sh_pfc_pin.

> >  struct sh_pfc_pin {
> >  	u16 pin;
> >  	u16 enum_id;
> > @@ -121,6 +125,8 @@ struct sh_pfc_soc_operations {
> >  	unsigned int (*get_bias)(struct sh_pfc *pfc, unsigned int pin);
> >  	void (*set_bias)(struct sh_pfc *pfc, unsigned int pin,
> >  			 unsigned int bias);
> > +	bool (*get_low_voltage)(struct sh_pfc *pfc, unsigned int pin);
> > +	void (*set_low_voltage)(struct sh_pfc *pfc, unsigned int pin, bool low);
> 
> [gs]et_io_voltage() or [gs]et_power_source() ?

I prefer the former.

Ben.

Re: [PATCH v3 3/6] pinctrl: sh-pfc: r8a7790: Implement voltage switching for SDHI

[Ben Hutchings]

On Mon, 2015-06-29 at 11:50 +0300, Laurent Pinchart wrote:
> Hi Ben,
> 
> Thank you for the patch.
> 
> On Friday 26 June 2015 16:23:30 Ben Hutchings wrote:
> > All the SHDIs can operate with either 3.3V or 1.8V signals, depending
> > on negotiation with the card.
> > 
> > Implement the {get,set}_low_voltage operations and set the low-voltage
> > capability flag for the associated 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 | 67 ++++++++++++++++++++++++++++++++-
> >  3 files changed, 68 insertions(+), 2
> > 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..38be7cbea4ca
> > 100644
> > --- a/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
> > +++ b/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
> > @@ -1739,10 +1739,20 @@ static const u16 pinmux_data[] = {
> >  #define PIN_NUMBER(r, c) (((r) - 'A') * 31 + (c) + 200)
> >  #define PIN_A_NUMBER(r, c) PIN_NUMBER(ROW_GROUP_A(r), c)
> > 
> > +#define PIN_LOW_VOLTAGE(bank, _pin, _name, sfx)		\
> > +	[RCAR_GP_PIN(bank, _pin)].configs = SH_PFC_PIN_CFG_LOW_VOLTAGE
> > +
> >  static const struct sh_pfc_pin pinmux_pins[] = {
> >  	PINMUX_GPIO_GP_ALL(),
> > 
> > -	/* Pins not associated with a GPIO port */
> > +	/*
> > +	 * All pins assigned to GPIO bank 3 can be used for SD interfaces
> > +	 * in which case they support low voltage (1.8V) signalling.
> > +	 */
> > +	PORT_GP_32(3, PIN_LOW_VOLTAGE, unused),
> 
> Ouch. I didn't know gcc would even support initializing fields of array member 
> structures separately from full initialization of the array member structure. 
> Is it standard C ? Is it supported by LLVM ? I would prefer replacing 
> PINMUX_GPIO_GP_ALL instead with a version that initializes the config field 
> appropriately.

It appears to be defined in the C99 standard.  (I only have the final
draft here, though.)

[...] 
> > +static bool sdhi_get_low_voltage(struct sh_pfc *pfc, unsigned int pin)
> > +{
> > +	void __iomem *mapped_reg;
> > +	u32 data, mask;
> > +
> > +	if (WARN(pin < RCAR_GP_PIN(3, 0) || pin > RCAR_GP_PIN(3, 31),
> > +		 "sdhi_get_low_voltage: invalid pin %#x", pin))
> > +		return 0;
> 
> If the rest of the driver behaves correctly this should never happen, right ?

Right.

> > +	/* Map IOCTRL6 */
> > +	mapped_reg = sh_pfc_phys_to_virt(pfc, 0xe606008c);
> > +
> > +	/* Bits in IOCTRL6 are numbered in opposite order to pins */
> > +	mask = 0x80000000 >> (pin & 0x1f);
> > +
> > +	data = sh_pfc_read_raw_reg(mapped_reg, 32);
> 
> Given that we know the register width here I would replace this with a direct 
> call to ioread32(). Same for the read and write calls below.
[...]

OK.

Ben.

Re: [PATCH v3 3/6] pinctrl: sh-pfc: r8a7790: Implement voltage switching for SDHI

[Ben Hutchings]

On Mon, 2015-06-29 at 11:50 +0300, Laurent Pinchart wrote:
> Hi Ben,
> 
> Thank you for the patch.
> 
> On Friday 26 June 2015 16:23:30 Ben Hutchings wrote:
> > All the SHDIs can operate with either 3.3V or 1.8V signals, depending
> > on negotiation with the card.
> > 
> > Implement the {get,set}_low_voltage operations and set the low-voltage
> > capability flag for the associated 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 | 67 ++++++++++++++++++++++++++++++++-
> >  3 files changed, 68 insertions(+), 2
> > 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..38be7cbea4ca
> > 100644
> > --- a/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
> > +++ b/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
> > @@ -1739,10 +1739,20 @@ static const u16 pinmux_data[] = {
> >  #define PIN_NUMBER(r, c) (((r) - 'A') * 31 + (c) + 200)
> >  #define PIN_A_NUMBER(r, c) PIN_NUMBER(ROW_GROUP_A(r), c)
> > 
> > +#define PIN_LOW_VOLTAGE(bank, _pin, _name, sfx)		\
> > +	[RCAR_GP_PIN(bank, _pin)].configs = SH_PFC_PIN_CFG_LOW_VOLTAGE
> > +
> >  static const struct sh_pfc_pin pinmux_pins[] = {
> >  	PINMUX_GPIO_GP_ALL(),
> > 
> > -	/* Pins not associated with a GPIO port */
> > +	/*
> > +	 * All pins assigned to GPIO bank 3 can be used for SD interfaces
> > +	 * in which case they support low voltage (1.8V) signalling.
> > +	 */
> > +	PORT_GP_32(3, PIN_LOW_VOLTAGE, unused),
> 
> Ouch. I didn't know gcc would even support initializing fields of array member 
> structures separately from full initialization of the array member structure. 
> Is it standard C ? Is it supported by LLVM ? I would prefer replacing 
> PINMUX_GPIO_GP_ALL instead with a version that initializes the config field 
> appropriately.

It appears to be defined in the C99 standard.  (I only have the final
draft here, though.)

[...] 
> > +static bool sdhi_get_low_voltage(struct sh_pfc *pfc, unsigned int pin)
> > +{
> > +	void __iomem *mapped_reg;
> > +	u32 data, mask;
> > +
> > +	if (WARN(pin < RCAR_GP_PIN(3, 0) || pin > RCAR_GP_PIN(3, 31),
> > +		 "sdhi_get_low_voltage: invalid pin %#x", pin))
> > +		return 0;
> 
> If the rest of the driver behaves correctly this should never happen, right ?

Right.

> > +	/* Map IOCTRL6 */
> > +	mapped_reg = sh_pfc_phys_to_virt(pfc, 0xe606008c);
> > +
> > +	/* Bits in IOCTRL6 are numbered in opposite order to pins */
> > +	mask = 0x80000000 >> (pin & 0x1f);
> > +
> > +	data = sh_pfc_read_raw_reg(mapped_reg, 32);
> 
> Given that we know the register width here I would replace this with a direct 
> call to ioread32(). Same for the read and write calls below.
[...]

OK.

Ben.

Re: [PATCH v3 5/6] ARM: shmobile: lager: Set clock rates for SDHI

[Ben Hutchings]

On Mon, 2015-06-29 at 06:23 +0000, Kuninori Morimoto wrote:
> Hi Ben 
> Cc Laurent, Geert, Magnus
> 
> > diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
> > index aaa4f258e279..5f68e53c58ae 100644
> > --- a/arch/arm/boot/dts/r8a7790-lager.dts
> > +++ b/arch/arm/boot/dts/r8a7790-lager.dts
> > @@ -488,6 +488,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 +501,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>;
> 
> Thank you for your patch, but I still wandering about this.
> Please correct me if I'm misunderstanding.
> 
> Above seetings is for SDHI IP, and it can divide it ?
> The image is
> 
> 	[CPG] -> 156 MHz -> [SDHI] -> 1/x -> [CARD]
> 	         ~~~~~~~
> 
> If so, why we can't use max-frequency ?
> We can calculate/set SDHI IP clocks via 
> max-frequency / clk_round_rate() / clk_set_rate()
> since we know SDHI's divider capability.
> 
> SH-MMC is using this style. and I think it is flexible for every speed.
> Please check sh_mmcif_clock_control(), sh_mmcif_clk_setup()
> on ${LINUX}/drivers/mmc/host/sh_mmcif.c

That's certainly a nicer way of doing this.  The difficulty I see is
that tmio_mmc doesn't know anything about the input clock, and not all
of the drivers using it actually use the clock framework.

Ben.

Re: [PATCH v3 5/6] ARM: shmobile: lager: Set clock rates for SDHI

[Ben Hutchings]

On Mon, 2015-06-29 at 06:23 +0000, Kuninori Morimoto wrote:
> Hi Ben 
> Cc Laurent, Geert, Magnus
> 
> > diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
> > index aaa4f258e279..5f68e53c58ae 100644
> > --- a/arch/arm/boot/dts/r8a7790-lager.dts
> > +++ b/arch/arm/boot/dts/r8a7790-lager.dts
> > @@ -488,6 +488,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 +501,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>;
> 
> Thank you for your patch, but I still wandering about this.
> Please correct me if I'm misunderstanding.
> 
> Above seetings is for SDHI IP, and it can divide it ?
> The image is
> 
> 	[CPG] -> 156 MHz -> [SDHI] -> 1/x -> [CARD]
> 	         ~~~~~~~
> 
> If so, why we can't use max-frequency ?
> We can calculate/set SDHI IP clocks via 
> max-frequency / clk_round_rate() / clk_set_rate()
> since we know SDHI's divider capability.
> 
> SH-MMC is using this style. and I think it is flexible for every speed.
> Please check sh_mmcif_clock_control(), sh_mmcif_clk_setup()
> on ${LINUX}/drivers/mmc/host/sh_mmcif.c

That's certainly a nicer way of doing this.  The difficulty I see is
that tmio_mmc doesn't know anything about the input clock, and not all
of the drivers using it actually use the clock framework.

Ben.

Re: [PATCH v3 5/6] ARM: shmobile: lager: Set clock rates for SDHI

[Ben Hutchings]

On Tue, 2015-06-30 at 01:02 +0100, Ben Hutchings wrote:
> On Mon, 2015-06-29 at 06:23 +0000, Kuninori Morimoto wrote:
[...]
> > If so, why we can't use max-frequency ?
> > We can calculate/set SDHI IP clocks via 
> > max-frequency / clk_round_rate() / clk_set_rate()
> > since we know SDHI's divider capability.
> > 
> > SH-MMC is using this style. and I think it is flexible for every speed.
> > Please check sh_mmcif_clock_control(), sh_mmcif_clk_setup()
> > on ${LINUX}/drivers/mmc/host/sh_mmcif.c
> 
> That's certainly a nicer way of doing this.  The difficulty I see is
> that tmio_mmc doesn't know anything about the input clock, and not all
> of the drivers using it actually use the clock framework.

More importantly, that algrithm can result in overclocking the card.

Ben.

Re: [PATCH v3 5/6] ARM: shmobile: lager: Set clock rates for SDHI

[Ben Hutchings]

On Tue, 2015-06-30 at 01:02 +0100, Ben Hutchings wrote:
> On Mon, 2015-06-29 at 06:23 +0000, Kuninori Morimoto wrote:
[...]
> > If so, why we can't use max-frequency ?
> > We can calculate/set SDHI IP clocks via 
> > max-frequency / clk_round_rate() / clk_set_rate()
> > since we know SDHI's divider capability.
> > 
> > SH-MMC is using this style. and I think it is flexible for every speed.
> > Please check sh_mmcif_clock_control(), sh_mmcif_clk_setup()
> > on ${LINUX}/drivers/mmc/host/sh_mmcif.c
> 
> That's certainly a nicer way of doing this.  The difficulty I see is
> that tmio_mmc doesn't know anything about the input clock, and not all
> of the drivers using it actually use the clock framework.

More importantly, that algrithm can result in overclocking the card.

Ben.

Re: [PATCH v3 5/6] ARM: shmobile: lager: Set clock rates for SDHI

[Ben Hutchings]

On Tue, 2015-06-30 at 02:29 +0100, Ben Hutchings wrote:
> On Tue, 2015-06-30 at 01:02 +0100, Ben Hutchings wrote:
> > On Mon, 2015-06-29 at 06:23 +0000, Kuninori Morimoto wrote:
> [...]
> > > If so, why we can't use max-frequency ?
> > > We can calculate/set SDHI IP clocks via 
> > > max-frequency / clk_round_rate() / clk_set_rate()
> > > since we know SDHI's divider capability.
> > > 
> > > SH-MMC is using this style. and I think it is flexible for every speed.
> > > Please check sh_mmcif_clock_control(), sh_mmcif_clk_setup()
> > > on ${LINUX}/drivers/mmc/host/sh_mmcif.c
> > 
> > That's certainly a nicer way of doing this.  The difficulty I see is
> > that tmio_mmc doesn't know anything about the input clock, and not all
> > of the drivers using it actually use the clock framework.
> 
> More importantly, that algrithm can result in overclocking the card.

...and some of the multiplications overflow!

Ben.

Re: [PATCH v3 5/6] ARM: shmobile: lager: Set clock rates for SDHI

[Ben Hutchings]

On Tue, 2015-06-30 at 02:29 +0100, Ben Hutchings wrote:
> On Tue, 2015-06-30 at 01:02 +0100, Ben Hutchings wrote:
> > On Mon, 2015-06-29 at 06:23 +0000, Kuninori Morimoto wrote:
> [...]
> > > If so, why we can't use max-frequency ?
> > > We can calculate/set SDHI IP clocks via 
> > > max-frequency / clk_round_rate() / clk_set_rate()
> > > since we know SDHI's divider capability.
> > > 
> > > SH-MMC is using this style. and I think it is flexible for every speed.
> > > Please check sh_mmcif_clock_control(), sh_mmcif_clk_setup()
> > > on ${LINUX}/drivers/mmc/host/sh_mmcif.c
> > 
> > That's certainly a nicer way of doing this.  The difficulty I see is
> > that tmio_mmc doesn't know anything about the input clock, and not all
> > of the drivers using it actually use the clock framework.
> 
> More importantly, that algrithm can result in overclocking the card.

...and some of the multiplications overflow!

Ben.

Re: [PATCH v3 5/6] ARM: shmobile: lager: Set clock rates for SDHI

[Kuninori Morimoto]

Hi Ben

> > > > SH-MMC is using this style. and I think it is flexible for every speed.
> > > > Please check sh_mmcif_clock_control(), sh_mmcif_clk_setup()
> > > > on ${LINUX}/drivers/mmc/host/sh_mmcif.c
> > > 
> > > That's certainly a nicer way of doing this.  The difficulty I see is
> > > that tmio_mmc doesn't know anything about the input clock, and not all
> > > of the drivers using it actually use the clock framework.
> > 
> > More importantly, that algrithm can result in overclocking the card.
> 
> ...and some of the multiplications overflow!

Can you show us more detail ?
It seems bug...

Best regards
---
Kuninori Morimoto

Re: [PATCH v3 5/6] ARM: shmobile: lager: Set clock rates for SDHI

[Ben Hutchings]

On Tue, 2015-06-30 at 01:45 +0000, Kuninori Morimoto wrote:
> Hi Ben
> 
> > > > > SH-MMC is using this style. and I think it is flexible for every speed.
> > > > > Please check sh_mmcif_clock_control(), sh_mmcif_clk_setup()
> > > > > on ${LINUX}/drivers/mmc/host/sh_mmcif.c
> > > > 
> > > > That's certainly a nicer way of doing this.  The difficulty I see is
> > > > that tmio_mmc doesn't know anything about the input clock, and not all
> > > > of the drivers using it actually use the clock framework.
> > > 
> > > More importantly, that algrithm can result in overclocking the card.
> > 
> > ...and some of the multiplications overflow!
> 
> Can you show us more detail ?
> It seems bug...

The requested clock rate can be up to 52 MHz and the divider can be up
to 1024.  With div = 1024 and clk = 52000000, clk * div overflows.

Ben.

Re: [PATCH v3 5/6] ARM: shmobile: lager: Set clock rates for SDHI

[Ben Hutchings]

On Tue, 2015-06-30 at 01:45 +0000, Kuninori Morimoto wrote:
> Hi Ben
> 
> > > > > SH-MMC is using this style. and I think it is flexible for every speed.
> > > > > Please check sh_mmcif_clock_control(), sh_mmcif_clk_setup()
> > > > > on ${LINUX}/drivers/mmc/host/sh_mmcif.c
> > > > 
> > > > That's certainly a nicer way of doing this.  The difficulty I see is
> > > > that tmio_mmc doesn't know anything about the input clock, and not all
> > > > of the drivers using it actually use the clock framework.
> > > 
> > > More importantly, that algrithm can result in overclocking the card.
> > 
> > ...and some of the multiplications overflow!
> 
> Can you show us more detail ?
> It seems bug...

The requested clock rate can be up to 52 MHz and the divider can be up
to 1024.  With div == 1024 and clk == 52000000, clk * div overflows.

Ben.

Re: [PATCH v3 5/6] ARM: shmobile: lager: Set clock rates for SDHI

[Kuninori Morimoto]

Hi Ben

> > Can you show us more detail ?
> > It seems bug...
> 
> The requested clock rate can be up to 52 MHz and the divider can be up
> to 1024.  With div = 1024 and clk = 52000000, clk * div overflows.

IMO, In such case, driver should find
 (input) 52 MHz / (divide) 1 = (request) 52 MHz
or similar.

52 GHz / 1024 = 52 MHz can be possible (from HW point),
but...

Re: [PATCH v3 5/6] ARM: shmobile: lager: Set clock rates for SDHI

[Kuninori Morimoto]

Hi Ben

> > Can you show us more detail ?
> > It seems bug...
> 
> The requested clock rate can be up to 52 MHz and the divider can be up
> to 1024.  With div == 1024 and clk == 52000000, clk * div overflows.

IMO, In such case, driver should find
 (input) 52 MHz / (divide) 1 = (request) 52 MHz
or similar.

52 GHz / 1024 = 52 MHz can be possible (from HW point),
but...