Skip to contentSkip to conversation

mmc: usdhi6rol0: UHS support

PATCH Superseded Lars Persson 3 participants 9 messages Apr 20, 2016 – Apr 21, 2016

Subsystems (3)
Device tree bindings MMC, SD and SDIO Arm/artpec machine support

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Lars Persson

author maintainer
#0 lore raw

v3 cover mmc: usdhi6rol0: UHS support

This patch set adds UHS support to the usdhi6rol0 driver, with control of the regulator and pin state settings.

Changes since v2:
- dt: renamed pins_uhs to state_uhs.
- dt: made default optional in pinctrl-names.
- dt: document that use of state_uhs also requires a default pin state.
- add error handling for missing pin states.

Changes since v1:
- Use mmc_regulator_set_vqmmc().

Lars Persson (3):
mmc: dt: usdhi6rol0: add optional pinctrl binding
mmc: usdhi6rol0: add support for UHS modes
mmc: usdhi6rol0: add pinctrl to set pin drive strength

 .../devicetree/bindings/mmc/usdhi6rol0.txt         |  6 +++
 drivers/mmc/host/usdhi6rol0.c                      | 62 ++++++++++++++++++++++
 2 files changed, 68 insertions(+)
Signature · 1 line
2.1.4
author maintainer
#1 lore raw

PATCH v3 1/3 mmc: dt: usdhi6rol0: add optional pinctrl binding

Add a pinctrl binding to specify different pin settings for high speed modes and UHS modes.

Signed-off-by Lars Persson <[email protected]>
---
 Documentation/devicetree/bindings/mmc/usdhi6rol0.txt | 6 ++++++
 1 file changed, 6 insertions(+)
Documentation/devicetree/bindings/mmc/usdhi6rol0.txt +6 -0
index 8babdaa..6d1b797 100644
@@ -12,6 +12,12 @@ Optional properties:
 
 - vmmc-supply:	a phandle of a regulator, supplying Vcc to the card
 - vqmmc-supply:	a phandle of a regulator, supplying VccQ to the card
+- pinctrl-names: Can contain a "default" entry and a "state_uhs"
+                 entry. The state_uhs entry is used together with the default
+                 entry when the board requires distinct settings for UHS speeds.
+
+- pinctrl-N: One property for each name listed in pinctrl-names, see
+             ../pinctrl/pinctrl-bindings.txt.
 
 Additionally any standard mmc bindings from mmc.txt can be used.
Signature · 1 line
2.1.4
author maintainer
#2 lore raw

PATCH v3 2/3 mmc: usdhi6rol0: add support for UHS modes

In reply to Lars Persson

Add a start_signal_voltage_switch() operation to support enabling of UHS modes.

Signed-off-by Lars Persson <[email protected]>
---
 drivers/mmc/host/usdhi6rol0.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)
drivers/mmc/host/usdhi6rol0.c +13 -0
index 807c06e..2585ea4 100644
@@ -1147,12 +1147,25 @@ static void usdhi6_enable_sdio_irq(struct mmc_host *mmc, int enable)
 	}
 }
 
+static int usdhi6_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+	int ret;
+
+	ret = mmc_regulator_set_vqmmc(mmc, ios);
+
+	if (ret < 0)
+		dev_warn(mmc_dev(mmc), "Voltage switch failed: %d\n", ret);
+
+	return ret;
+}
+
 static struct mmc_host_ops usdhi6_ops = {
 	.request	= usdhi6_request,
 	.set_ios	= usdhi6_set_ios,
 	.get_cd		= usdhi6_get_cd,
 	.get_ro		= usdhi6_get_ro,
 	.enable_sdio_irq = usdhi6_enable_sdio_irq,
+	.start_signal_voltage_switch = usdhi6_sig_volt_switch,
 };
 
 /*			State machine handlers				*/
Signature · 1 line
2.1.4
author maintainer
#3 lore raw

Re: [PATCH v3 2/3] mmc: usdhi6rol0: add support for UHS modes

Quoted 13 lines · Lars Persson

Add a start_signal_voltage_switch() operation to support enabling of UHS modes.

Signed-off-by Lars Persson <[email protected]>
---
  drivers/mmc/host/usdhi6rol0.c | 13 +++++++++++++
  1 file changed, 13 insertions(+)
drivers/mmc/host/usdhi6rol0.c +0 -0
index 807c06e..2585ea4 100644

drivers/mmc/host/usdhi6rol0.c static void usdhi6_enable_sdio_irq(struct mmc_host *mmc, int enable) · line 1153

Quoted 7 lines
drivers/mmc/host/usdhi6rol0.c +4 -0
@@ -1147,12 +1147,25 @@ static void usdhi6_enable_sdio_irq(struct mmc_host *mmc, int enable)
  	}
  }
 
+static int usdhi6_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+	int ret;
+

To support boards without regulator we need to put a IS_ERR_OR_NULL(mmc->supply.vqmmc) check here and bail out with return 0. Missing vqmmc is OK according to our binding document, but treated as an error by mmc_regulator_set_vqmmc().

Quoted 19 lines
drivers/mmc/host/usdhi6rol0.c +9 -0
+	ret = mmc_regulator_set_vqmmc(mmc, ios);
+
+	if (ret < 0)
+		dev_warn(mmc_dev(mmc), "Voltage switch failed: %d\n", ret);
+
+	return ret;
+}
+
  static struct mmc_host_ops usdhi6_ops = {
  	.request	= usdhi6_request,
  	.set_ios	= usdhi6_set_ios,
  	.get_cd		= usdhi6_get_cd,
  	.get_ro		= usdhi6_get_ro,
  	.enable_sdio_irq = usdhi6_enable_sdio_irq,
+	.start_signal_voltage_switch = usdhi6_sig_volt_switch,
  };
 
  /*			State machine handlers				*/
#4 lore raw

Re: [PATCH v3 2/3] mmc: usdhi6rol0: add support for UHS modes

Quoted 17 lines · On Thursday 21 April 2016 09:12:52 Lars Persson wrote:
Quoted 16 lines · Lars Persson

Add a start_signal_voltage_switch() operation to support enabling of UHS modes.

Signed-off-by Lars Persson <[email protected]>
---
  drivers/mmc/host/usdhi6rol0.c | 13 +++++++++++++
  1 file changed, 13 insertions(+)
drivers/mmc/host/usdhi6rol0.c +0 -0
index 807c06e..2585ea4 100644
@@ -1147,12 +1147,25 @@ static void usdhi6_enable_sdio_irq(struct mmc_host *mmc, int enable)
      }
  }
Quoted 9 lines

drivers/mmc/host/usdhi6rol0.c static void usdhi6_enable_sdio_irq(struct mmc_host *mmc, int enable) · line 1153

Quoted 4 lines
drivers/mmc/host/usdhi6rol0.c +4 -0
+static int usdhi6_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+     int ret;
+

To support boards without regulator we need to put a IS_ERR_OR_NULL(mmc->supply.vqmmc) check here and bail out with return 0. Missing vqmmc is OK according to our binding document, but treated as an error by mmc_regulator_set_vqmmc().

mmc_regulator_set_vqmmc() should already take care of handling all the those cases. In general, it's almost always a bug to use IS_ERR_OR_NULL(): if you ever feel the need to use it, you should either fix the code that made the pointer ambiguous or you have misunderstood the interface.

Arnd

#5 lore raw

Re: [PATCH v3 2/3] mmc: usdhi6rol0: add support for UHS modes

Quoted 28 lines · Arnd Bergmann
Quoted 17 lines · On Thursday 21 April 2016 09:12:52 Lars Persson wrote:
Quoted 16 lines · Lars Persson

Add a start_signal_voltage_switch() operation to support enabling of UHS modes.

Signed-off-by Lars Persson <[email protected]>
---
   drivers/mmc/host/usdhi6rol0.c | 13 +++++++++++++
   1 file changed, 13 insertions(+)
drivers/mmc/host/usdhi6rol0.c +0 -0
index 807c06e..2585ea4 100644
@@ -1147,12 +1147,25 @@ static void usdhi6_enable_sdio_irq(struct mmc_host *mmc, int enable)
       }
   }
Quoted 9 lines

drivers/mmc/host/usdhi6rol0.c static void usdhi6_enable_sdio_irq(struct mmc_host *mmc, int enable) · line 1153

Quoted 4 lines
drivers/mmc/host/usdhi6rol0.c +4 -0
+static int usdhi6_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+     int ret;
+

To support boards without regulator we need to put a IS_ERR_OR_NULL(mmc->supply.vqmmc) check here and bail out with return 0. Missing vqmmc is OK according to our binding document, but treated as an error by mmc_regulator_set_vqmmc().

Quoted 8 lines

mmc_regulator_set_vqmmc() should already take care of handling all the those cases. In general, it's almost always a bug to use IS_ERR_OR_NULL(): if you ever feel the need to use it, you should either fix the code that made the pointer ambiguous or you have misunderstood the interface.

Arnd

Yes it should of course be IS_ERR() rather than IS_ERR_OR_NULL().

It is a common pattern in all upstream mmc drivers to not make the call to mmc_regulator_set_vqmmc() when a regulator is missing. We basically want to log errors except for the EINVAL returned in case of a missing regulator.

- Lars
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#6 lore raw

Re: [PATCH v3 2/3] mmc: usdhi6rol0: add support for UHS modes

Quoted 7 lines · On Thursday 21 April 2016 10:26:55 Lars Persson wrote:

It is a common pattern in all upstream mmc drivers to not make the call to mmc_regulator_set_vqmmc() when a regulator is missing. We basically want to log errors except for the EINVAL returned in case of a missing regulator.

I see what you mean here: two out of four drivers calling mmc_regulator_set_vqmmc() log errors to the console.

If we want all drivers to behave consistently here, how about moving that error output into the mmc_regulator_set_vqmmc() function itself?

Arnd

author maintainer
#7 lore raw

Re: [PATCH v3 2/3] mmc: usdhi6rol0: add support for UHS modes

Quoted 9 lines · Arnd Bergmann
Quoted 7 lines · On Thursday 21 April 2016 10:26:55 Lars Persson wrote:

It is a common pattern in all upstream mmc drivers to not make the call to mmc_regulator_set_vqmmc() when a regulator is missing. We basically want to log errors except for the EINVAL returned in case of a missing regulator.

Quoted 7 lines

I see what you mean here: two out of four drivers calling mmc_regulator_set_vqmmc() log errors to the console.

If we want all drivers to behave consistently here, how about moving that error output into the mmc_regulator_set_vqmmc() function itself?

I will make a v4 that skips the error output and if Ulf agrees with your proposal I will make another patch series to move the dev_dbg() into mmc_regulator_set_vqmmc().

- Lars

author maintainer
#8 lore raw

PATCH v3 3/3 mmc: usdhi6rol0: add pinctrl to set pin drive strength

In reply to Lars Persson

Some boards need different pin drive strength for the UHS mode. Add an optional pinctrl setting with two pin states covering UHS speeds and other speeds.

Signed-off-by Lars Persson <[email protected]>
---
 drivers/mmc/host/usdhi6rol0.c | 51 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)
drivers/mmc/host/usdhi6rol0.c +50 -1
index 2585ea4..c4d923b 100644
@@ -22,6 +22,7 @@
 #include <linux/mmc/sdio.h>
 #include <linux/module.h>
 #include <linux/pagemap.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/scatterlist.h>
 #include <linux/string.h>

@@ -198,6 +199,11 @@ struct usdhi6_host {
 	struct dma_chan *chan_rx;
 	struct dma_chan *chan_tx;
 	bool dma_active;
+
+	/* Pin control */
+	struct pinctrl *pinctrl;
+	struct pinctrl_state *pins_default;
+	struct pinctrl_state *pins_uhs;
 };
 
 /*			I/O primitives					*/

@@ -1147,14 +1153,38 @@ static void usdhi6_enable_sdio_irq(struct mmc_host *mmc, int enable)
 	}
 }
 
+static int usdhi6_set_pinstates(struct usdhi6_host *host, int voltage)
+{
+	if (IS_ERR(host->pins_uhs))
+		return 0;
+
+	switch (voltage) {
+	case MMC_SIGNAL_VOLTAGE_180:
+	case MMC_SIGNAL_VOLTAGE_120:
+		return pinctrl_select_state(host->pinctrl,
+					    host->pins_uhs);
+
+	default:
+		return pinctrl_select_state(host->pinctrl,
+					    host->pins_default);
+	}
+}
+
 static int usdhi6_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	int ret;
 
 	ret = mmc_regulator_set_vqmmc(mmc, ios);
 
-	if (ret < 0)
+	if (ret < 0) {
 		dev_warn(mmc_dev(mmc), "Voltage switch failed: %d\n", ret);
+		return ret;
+	}
+
+	ret = usdhi6_set_pinstates(mmc_priv(mmc), ios->signal_voltage);
+	if (ret)
+		dev_warn_once(mmc_dev(mmc),
+			      "Failed to set pinstate err=%d\n", ret);
 
 	return ret;
 }

@@ -1743,6 +1773,25 @@ static int usdhi6_probe(struct platform_device *pdev)
 	host->wait	= USDHI6_WAIT_FOR_REQUEST;
 	host->timeout	= msecs_to_jiffies(4000);
 
+	host->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (IS_ERR(host->pinctrl)) {
+		ret = PTR_ERR(host->pinctrl);
+		goto e_free_mmc;
+	}
+
+	host->pins_uhs = pinctrl_lookup_state(host->pinctrl, "state_uhs");
+	if (!IS_ERR(host->pins_uhs)) {
+		host->pins_default = pinctrl_lookup_state(host->pinctrl,
+							  PINCTRL_STATE_DEFAULT);
+
+		if (IS_ERR(host->pins_default)) {
+			dev_err(dev,
+				"UHS pinctrl requires a default pin state.\n");
+			ret = PTR_ERR(host->pins_default);
+			goto e_free_mmc;
+		}
+	}
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	host->base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(host->base)) {
Signature · 1 line
2.1.4