Re: [PATCH 13/41] mmc: host: omap_hsmmc: Add support to set IODELAY values

[Rob Herring]

On Fri, May 19, 2017 at 01:45:13PM +0530, Kishon Vijay Abraham I wrote:
> The data manual of J6/J6 Eco recommends to set different IODELAY values
> depending on the mode in which the MMC/SD is enumerated in order to
> ensure IO timings are met.
> 
> Add support to set the IODELAY values depending on the various MMC
> modes using the pinctrl APIs.
> 
> Signed-off-by: Kishon Vijay Abraham I 
> [nsek...@ti.com: introduce OMAP_HSMMC_SETUP_PINCTRL()]
> Signed-off-by: Sekhar Nori 
> ---
>  .../devicetree/bindings/mmc/ti-omap-hsmmc.txt  |   5 +
>  drivers/mmc/host/omap_hsmmc.c  | 124 
> -
>  include/linux/platform_data/hsmmc-omap.h   |   3 +
>  3 files changed, 129 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt 
> b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> index 258e25af10f7..dcf0b777c031 100644
> --- a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> @@ -20,6 +20,11 @@ Optional properties:
>  ti,dual-volt: boolean, supports dual voltage cards
>  -supply: phandle to the regulator device tree node
>  "supply-name" examples are "vmmc", "vmmc_aux" etc
> +pinctrl-names: Should be a list of pinctrl state names and can be "sdr104",
> +"hs200_1_8v", "ddr50", "sdr50", "sdr25", "sdr12", "hs", "ddr_1_8v" or
> +"default".
> +pinctrl-: Phandle referencing pin configuration of the sd/emmc 
> controller.
> +See: Documentation/devicetree/bindings/pinctrl/pinctrl-binding.txt
>  ti,non-removable: non-removable slot (like eMMC)
>  ti,needs-special-reset: Requires a special softreset sequence
>  ti,needs-special-hs-handling: HSMMC IP needs special setting for handling 
> High Speed

Binding looks fine, but...

> @@ -2388,6 +2427,73 @@ static inline struct omap_hsmmc_platform_data
>  }
>  #endif
>  
> +#define OMAP_HSMMC_SETUP_PINCTRL(capvar, capmask, mode)  
> \
> + do {\

This is kind of ugly. Why not make it a function. Just return 
pinctrl_state ptr.

> + struct pinctrl_state *s = ERR_PTR(-ENODEV); \
> + char str[20];   \
> + char *version = host->pdata->version;   \
> + \
> + if (!(mmc->capvar & (capmask))) \
> + break;  \
> + \
> + if (host->pdata->version) { \
> + sprintf(str, "%s-%s", #mode, version);  \
> + s = pinctrl_lookup_state(host->pinctrl, str);   \
> + }   \
> + \
> + if (IS_ERR(s)) {\
> + sprintf(str, "%s", #mode);  \
> + s = pinctrl_lookup_state(host->pinctrl, str);   \
> + }   \
> + \
> + if (IS_ERR(s)) {\
> + dev_err(host->dev, "no pinctrl state for %s "   \
> + "mode\n", #mode);   \
> + mmc->capvar &= ~(capmask);  \
> + } else {\
> + host->mode##_pinctrl_state = s; \
> + }   \
> + \
> + } while (0)
> +
> +static int omap_hsmmc_get_iodelay_pinctrl_state(struct omap_hsmmc_host *host)
> +{
> + struct mmc_host *mmc = host->mmc;
> +
> + if (!(host->pdata->controller_flags & OMAP_HSMMC_REQUIRE_IODELAY))
> + return 0;
> +
> + host->pinctrl = devm_pinctrl_get(host->dev);
> + if (IS_ERR(host->pinctrl)) {
> + dev_err(host->dev, "Cannot get pinctrl\n");
> + return PTR_ERR(host->pinctrl);
> + }
> +
> + host->default_pinctrl_state = pinctrl_lookup_state(host->pinctrl,
> +"default");
> + if (IS_ERR(host->default_pinctrl_state)) {
> + dev_err(host->dev,
> + "no pinctrl state for default mode\n");
> + return PTR_ERR(host->default_pinctrl_state);
> + }
> +
> + OMAP_HSMMC_SETUP_PINCTRL(caps,  MMC_CAP_UHS_SDR104, sdr104);
> + OMAP_HSMMC_SETUP_PINCTRL(caps,  MMC_CAP_UHS_DDR50, 

[PATCH 13/41] mmc: host: omap_hsmmc: Add support to set IODELAY values

[Kishon Vijay Abraham I]

The data manual of J6/J6 Eco recommends to set different IODELAY values
depending on the mode in which the MMC/SD is enumerated in order to
ensure IO timings are met.

Add support to set the IODELAY values depending on the various MMC
modes using the pinctrl APIs.

Signed-off-by: Kishon Vijay Abraham I 
[nsek...@ti.com: introduce OMAP_HSMMC_SETUP_PINCTRL()]
Signed-off-by: Sekhar Nori 
---
 .../devicetree/bindings/mmc/ti-omap-hsmmc.txt  |   5 +
 drivers/mmc/host/omap_hsmmc.c  | 124 -
 include/linux/platform_data/hsmmc-omap.h   |   3 +
 3 files changed, 129 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt 
b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
index 258e25af10f7..dcf0b777c031 100644
--- a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
+++ b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
@@ -20,6 +20,11 @@ Optional properties:
 ti,dual-volt: boolean, supports dual voltage cards
 -supply: phandle to the regulator device tree node
 "supply-name" examples are "vmmc", "vmmc_aux" etc
+pinctrl-names: Should be a list of pinctrl state names and can be "sdr104",
+"hs200_1_8v", "ddr50", "sdr50", "sdr25", "sdr12", "hs", "ddr_1_8v" or
+"default".
+pinctrl-: Phandle referencing pin configuration of the sd/emmc controller.
+See: Documentation/devicetree/bindings/pinctrl/pinctrl-binding.txt
 ti,non-removable: non-removable slot (like eMMC)
 ti,needs-special-reset: Requires a special softreset sequence
 ti,needs-special-hs-handling: HSMMC IP needs special setting for handling High 
Speed
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index b28f0e9631ce..7271c7e3144c 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -258,6 +258,18 @@ struct omap_hsmmc_host {
struct timer_list   timer;
unsigned long long  data_timeout;
 
+   struct pinctrl  *pinctrl;
+   struct pinctrl_state*pinctrl_state;
+   struct pinctrl_state*default_pinctrl_state;
+   struct pinctrl_state*sdr104_pinctrl_state;
+   struct pinctrl_state*hs200_1_8v_pinctrl_state;
+   struct pinctrl_state*ddr50_pinctrl_state;
+   struct pinctrl_state*sdr50_pinctrl_state;
+   struct pinctrl_state*sdr25_pinctrl_state;
+   struct pinctrl_state*sdr12_pinctrl_state;
+   struct pinctrl_state*hs_pinctrl_state;
+   struct pinctrl_state*ddr_1_8v_pinctrl_state;
+
/* return MMC cover switch state, can be NULL if not supported.
 *
 * possible return values:
@@ -1729,6 +1741,8 @@ static void omap_hsmmc_request(struct mmc_host *mmc, 
struct mmc_request *req)
 static void omap_hsmmc_set_timing(struct omap_hsmmc_host *host)
 {
u32 val;
+   int ret;
+   struct pinctrl_state *pinctrl_state;
struct mmc_ios *ios = >mmc->ios;
 
omap_hsmmc_stop_clock(host);
@@ -1738,35 +1752,54 @@ static void omap_hsmmc_set_timing(struct 
omap_hsmmc_host *host)
switch (ios->timing) {
case MMC_TIMING_UHS_SDR104:
val |= AC12_UHSMC_SDR104;
+   pinctrl_state = host->sdr104_pinctrl_state;
break;
case MMC_TIMING_MMC_HS200:
val |= AC12_UHSMC_SDR104;
+   pinctrl_state = host->hs200_1_8v_pinctrl_state;
break;
case MMC_TIMING_UHS_DDR50:
val |= AC12_UHSMC_DDR50;
+   pinctrl_state = host->ddr50_pinctrl_state;
break;
case MMC_TIMING_UHS_SDR50:
val |= AC12_UHSMC_SDR50;
+   pinctrl_state = host->sdr50_pinctrl_state;
break;
case MMC_TIMING_UHS_SDR25:
val |= AC12_UHSMC_SDR25;
+   pinctrl_state = host->sdr25_pinctrl_state;
break;
case MMC_TIMING_UHS_SDR12:
val |= AC12_UHSMC_SDR12;
+   pinctrl_state = host->sdr12_pinctrl_state;
break;
case MMC_TIMING_SD_HS:
case MMC_TIMING_MMC_HS:
val |= AC12_UHSMC_RES;
+   pinctrl_state = host->hs_pinctrl_state;
break;
case MMC_TIMING_MMC_DDR52:
val |= AC12_UHSMC_RES;
+   pinctrl_state = host->ddr_1_8v_pinctrl_state;
break;
default:
val |= AC12_UHSMC_RES;
+   pinctrl_state = host->default_pinctrl_state;
break;
}
OMAP_HSMMC_WRITE(host->base, AC12, val);
 
+   if (host->pdata->controller_flags & OMAP_HSMMC_REQUIRE_IODELAY) {
+   ret = pinctrl_select_state(host->pinctrl, pinctrl_state);
+   if (ret) {
+   dev_err(mmc_dev(host->mmc),
+   "failed to select pinctrl state\n");
+   return;
+   }
+