| Message ID | 1378299257-2980-6-git-send-email-b29396@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 04, 2013 at 08:54:14PM +0800, Dong Aisheng wrote: > Without proper pinctrl state, the card may not be able to work > on high speed stablely. e.g. SDR104. > > This patch add pinctrl state switch code according to different > uhs mode include 100mhz sate, 200mhz sate and normal state > (50Mhz and below). > > Signed-off-by: Dong Aisheng <b29396@freescale.com> > --- > drivers/mmc/host/sdhci-esdhc-imx.c | 110 ++++++++++++++++++++++++++- > include/linux/platform_data/mmc-esdhc-imx.h | 4 + > 2 files changed, 113 insertions(+), 1 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c > index 36b9f63..3e89863 100644 > --- a/drivers/mmc/host/sdhci-esdhc-imx.c > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c > @@ -49,6 +49,10 @@ > > #define ESDHC_TUNING_BLOCK_PATTERN_LEN 64 > > +/* pinctrl state */ > +#define ESDHC_PINCTRL_STATE_100MHZ "state_100mhz" > +#define ESDHC_PINCTRL_STATE_200MHZ "state_200mhz" > + > /* > * Our interpretation of the SDHCI_HOST_CONTROL register > */ > @@ -90,6 +94,10 @@ struct pltfm_imx_data { > u32 scratchpad; > enum imx_esdhc_type devtype; > struct pinctrl *pinctrl; > + struct pinctrl_state *pins_current; > + struct pinctrl_state *pins_default; > + struct pinctrl_state *pins_100mhz; > + struct pinctrl_state *pins_200mhz; > struct esdhc_platform_data boarddata; > struct clk *clk_ipg; > struct clk *clk_ahb; > @@ -642,6 +650,75 @@ static int esdhc_executing_tuning(struct sdhci_host *host, u32 opcode) > return ret; > } > > +static int esdhc_change_pinstate(struct sdhci_host *host, > + unsigned int uhs) > +{ > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct pltfm_imx_data *imx_data = pltfm_host->priv; > + struct pinctrl_state *pinctrl; > + int ret; > + > + dev_dbg(mmc_dev(host->mmc), "change pinctrl state for uhs %d\n", uhs); > + > + if (IS_ERR(imx_data->pinctrl) || > + IS_ERR(imx_data->pins_default) || > + IS_ERR(imx_data->pins_100mhz) || > + IS_ERR(imx_data->pins_200mhz)) > + return -EINVAL; > + > + switch (uhs) { > + case MMC_TIMING_UHS_SDR12: > + case MMC_TIMING_UHS_SDR25: > + case MMC_TIMING_UHS_DDR50: > + pinctrl = imx_data->pins_default; > + break; > + case MMC_TIMING_UHS_SDR50: > + pinctrl = imx_data->pins_100mhz; > + break; > + case MMC_TIMING_UHS_SDR104: > + pinctrl = imx_data->pins_200mhz; > + break; > + default: > + /* back to default state for other legacy timing */ > + pinctrl = imx_data->pins_default; > + } So it can be equally written as below? switch (uhs) { case MMC_TIMING_UHS_SDR104: pinctrl = imx_data->pins_200mhz; break; case MMC_TIMING_UHS_SDR50: pinctrl = imx_data->pins_100mhz; break; default: pinctrl = imx_data->pins_default; } > + > + if (pinctrl == imx_data->pins_current) > + return 0; I think pinctrl_select_state() already take care of the check? > + > + ret = pinctrl_select_state(imx_data->pinctrl, pinctrl); > + if (!ret) > + imx_data->pins_current = pinctrl; > + > + return ret; > +} > + > +static int esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) > +{ > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct pltfm_imx_data *imx_data = pltfm_host->priv; > + > + switch (uhs) { > + case MMC_TIMING_UHS_SDR12: > + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR12; > + break; > + case MMC_TIMING_UHS_SDR25: > + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR25; > + break; > + case MMC_TIMING_UHS_SDR50: > + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR50; > + break; > + case MMC_TIMING_UHS_SDR104: > + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR104; > + break; > + case MMC_TIMING_UHS_DDR50: > + imx_data->uhs_mode = SDHCI_CTRL_UHS_DDR50; > + break; > + } > + > + return esdhc_change_pinstate(host, uhs); > +} > + > static const struct sdhci_ops sdhci_esdhc_ops = { > .read_l = esdhc_readl_le, > .read_w = esdhc_readw_le, > @@ -653,6 +730,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = { > .get_min_clock = esdhc_pltfm_get_min_clock, > .get_ro = esdhc_pltfm_get_ro, > .platform_bus_width = esdhc_pltfm_bus_width, > + .set_uhs_signaling = esdhc_set_uhs_signaling, > .platform_execute_tuning = esdhc_executing_tuning, > }; > > @@ -695,6 +773,11 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev, > > of_property_read_u32(np, "max-frequency", &boarddata->f_max); > > + if (of_find_property(np, "no-1-8-v", NULL)) > + boarddata->support_vsel = false; > + else > + boarddata->support_vsel = true; So you check "no-1-8-v" property, set support_vsel here and then set SDHCI_QUIRK2_NO_1_8_V accordingly below in sdhci_esdhc_imx_probe(). But sdhci_get_of_property() - sdhci-pltfm.c already does the job. Shawn > + > return 0; > } > #else > @@ -757,12 +840,20 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) > clk_prepare_enable(imx_data->clk_ipg); > clk_prepare_enable(imx_data->clk_ahb); > > - imx_data->pinctrl = devm_pinctrl_get_select_default(&pdev->dev); > + imx_data->pinctrl = devm_pinctrl_get(&pdev->dev); > if (IS_ERR(imx_data->pinctrl)) { > err = PTR_ERR(imx_data->pinctrl); > goto disable_clk; > } > > + imx_data->pins_default = pinctrl_lookup_state(imx_data->pinctrl, > + PINCTRL_STATE_DEFAULT); > + if (IS_ERR(imx_data->pins_default)) { > + err = PTR_ERR(imx_data->pins_default); > + dev_err(mmc_dev(host->mmc), "could not get default state\n"); > + goto disable_clk; > + } > + > host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL; > > if (is_imx25_esdhc(imx_data) || is_imx35_esdhc(imx_data)) > @@ -839,6 +930,23 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) > break; > } > > + /* sdr50 and sdr104 needs work on 1.8v signal voltage */ > + if ((boarddata->support_vsel) && is_imx6q_usdhc(imx_data)) { > + imx_data->pins_100mhz = pinctrl_lookup_state(imx_data->pinctrl, > + ESDHC_PINCTRL_STATE_100MHZ); > + imx_data->pins_200mhz = pinctrl_lookup_state(imx_data->pinctrl, > + ESDHC_PINCTRL_STATE_200MHZ); > + if (IS_ERR(imx_data->pins_100mhz) || > + IS_ERR(imx_data->pins_200mhz)) { > + dev_warn(mmc_dev(host->mmc), > + "could not get ultra high speed state, work on normal mode\n"); > + /* fall back to not support uhs by specify no 1.8v quirk */ > + host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; > + } > + } else { > + host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; > + } > + > err = sdhci_add_host(host); > if (err) > goto disable_clk; > diff --git a/include/linux/platform_data/mmc-esdhc-imx.h b/include/linux/platform_data/mmc-esdhc-imx.h > index d44912d..a0f5a8f 100644 > --- a/include/linux/platform_data/mmc-esdhc-imx.h > +++ b/include/linux/platform_data/mmc-esdhc-imx.h > @@ -10,6 +10,8 @@ > #ifndef __ASM_ARCH_IMX_ESDHC_H > #define __ASM_ARCH_IMX_ESDHC_H > > +#include <linux/types.h> > + > enum wp_types { > ESDHC_WP_NONE, /* no WP, neither controller nor gpio */ > ESDHC_WP_CONTROLLER, /* mmc controller internal WP */ > @@ -32,6 +34,7 @@ enum cd_types { > * @cd_gpio: gpio for card_detect interrupt > * @wp_type: type of write_protect method (see wp_types enum above) > * @cd_type: type of card_detect method (see cd_types enum above) > + * @support_vsel: indicate it supports 1.8v switching > */ > > struct esdhc_platform_data { > @@ -41,5 +44,6 @@ struct esdhc_platform_data { > enum cd_types cd_type; > int max_bus_width; > unsigned int f_max; > + bool support_vsel; > }; > #endif /* __ASM_ARCH_IMX_ESDHC_H */ > -- > 1.7.1 > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4 September 2013 14:54, Dong Aisheng <b29396@freescale.com> wrote: > Without proper pinctrl state, the card may not be able to work > on high speed stablely. e.g. SDR104. > > This patch add pinctrl state switch code according to different > uhs mode include 100mhz sate, 200mhz sate and normal state > (50Mhz and below). > > Signed-off-by: Dong Aisheng <b29396@freescale.com> > --- > drivers/mmc/host/sdhci-esdhc-imx.c | 110 ++++++++++++++++++++++++++- > include/linux/platform_data/mmc-esdhc-imx.h | 4 + > 2 files changed, 113 insertions(+), 1 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c > index 36b9f63..3e89863 100644 > --- a/drivers/mmc/host/sdhci-esdhc-imx.c > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c > @@ -49,6 +49,10 @@ > > #define ESDHC_TUNING_BLOCK_PATTERN_LEN 64 > > +/* pinctrl state */ > +#define ESDHC_PINCTRL_STATE_100MHZ "state_100mhz" > +#define ESDHC_PINCTRL_STATE_200MHZ "state_200mhz" > + > /* > * Our interpretation of the SDHCI_HOST_CONTROL register > */ > @@ -90,6 +94,10 @@ struct pltfm_imx_data { > u32 scratchpad; > enum imx_esdhc_type devtype; > struct pinctrl *pinctrl; > + struct pinctrl_state *pins_current; > + struct pinctrl_state *pins_default; > + struct pinctrl_state *pins_100mhz; > + struct pinctrl_state *pins_200mhz; > struct esdhc_platform_data boarddata; > struct clk *clk_ipg; > struct clk *clk_ahb; > @@ -642,6 +650,75 @@ static int esdhc_executing_tuning(struct sdhci_host *host, u32 opcode) > return ret; > } > > +static int esdhc_change_pinstate(struct sdhci_host *host, > + unsigned int uhs) > +{ > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct pltfm_imx_data *imx_data = pltfm_host->priv; > + struct pinctrl_state *pinctrl; > + int ret; > + > + dev_dbg(mmc_dev(host->mmc), "change pinctrl state for uhs %d\n", uhs); > + > + if (IS_ERR(imx_data->pinctrl) || > + IS_ERR(imx_data->pins_default) || > + IS_ERR(imx_data->pins_100mhz) || > + IS_ERR(imx_data->pins_200mhz)) > + return -EINVAL; > + > + switch (uhs) { > + case MMC_TIMING_UHS_SDR12: > + case MMC_TIMING_UHS_SDR25: > + case MMC_TIMING_UHS_DDR50: > + pinctrl = imx_data->pins_default; > + break; > + case MMC_TIMING_UHS_SDR50: > + pinctrl = imx_data->pins_100mhz; > + break; > + case MMC_TIMING_UHS_SDR104: > + pinctrl = imx_data->pins_200mhz; > + break; > + default: > + /* back to default state for other legacy timing */ > + pinctrl = imx_data->pins_default; > + } > + > + if (pinctrl == imx_data->pins_current) > + return 0; > + > + ret = pinctrl_select_state(imx_data->pinctrl, pinctrl); > + if (!ret) > + imx_data->pins_current = pinctrl; > + > + return ret; > +} > + > +static int esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) > +{ > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct pltfm_imx_data *imx_data = pltfm_host->priv; > + > + switch (uhs) { > + case MMC_TIMING_UHS_SDR12: > + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR12; > + break; > + case MMC_TIMING_UHS_SDR25: > + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR25; > + break; > + case MMC_TIMING_UHS_SDR50: > + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR50; > + break; > + case MMC_TIMING_UHS_SDR104: > + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR104; > + break; > + case MMC_TIMING_UHS_DDR50: > + imx_data->uhs_mode = SDHCI_CTRL_UHS_DDR50; > + break; > + } > + > + return esdhc_change_pinstate(host, uhs); > +} > + > static const struct sdhci_ops sdhci_esdhc_ops = { > .read_l = esdhc_readl_le, > .read_w = esdhc_readw_le, > @@ -653,6 +730,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = { > .get_min_clock = esdhc_pltfm_get_min_clock, > .get_ro = esdhc_pltfm_get_ro, > .platform_bus_width = esdhc_pltfm_bus_width, > + .set_uhs_signaling = esdhc_set_uhs_signaling, > .platform_execute_tuning = esdhc_executing_tuning, > }; > > @@ -695,6 +773,11 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev, > > of_property_read_u32(np, "max-frequency", &boarddata->f_max); > > + if (of_find_property(np, "no-1-8-v", NULL)) > + boarddata->support_vsel = false; > + else > + boarddata->support_vsel = true; > + > return 0; > } > #else > @@ -757,12 +840,20 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) > clk_prepare_enable(imx_data->clk_ipg); > clk_prepare_enable(imx_data->clk_ahb); > > - imx_data->pinctrl = devm_pinctrl_get_select_default(&pdev->dev); > + imx_data->pinctrl = devm_pinctrl_get(&pdev->dev); > if (IS_ERR(imx_data->pinctrl)) { > err = PTR_ERR(imx_data->pinctrl); > goto disable_clk; > } > > + imx_data->pins_default = pinctrl_lookup_state(imx_data->pinctrl, > + PINCTRL_STATE_DEFAULT); I believe you should look into the new pinctrl APIs and the new sequence at device driver core probe, which automatically fetches default, idle and sleep state pins. Additionally it sets the default state. It should likely simplifies some code in this patch. Kind regards Ulf Hansson > + if (IS_ERR(imx_data->pins_default)) { > + err = PTR_ERR(imx_data->pins_default); > + dev_err(mmc_dev(host->mmc), "could not get default state\n"); > + goto disable_clk; > + } > + > host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL; > > if (is_imx25_esdhc(imx_data) || is_imx35_esdhc(imx_data)) > @@ -839,6 +930,23 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) > break; > } > > + /* sdr50 and sdr104 needs work on 1.8v signal voltage */ > + if ((boarddata->support_vsel) && is_imx6q_usdhc(imx_data)) { > + imx_data->pins_100mhz = pinctrl_lookup_state(imx_data->pinctrl, > + ESDHC_PINCTRL_STATE_100MHZ); > + imx_data->pins_200mhz = pinctrl_lookup_state(imx_data->pinctrl, > + ESDHC_PINCTRL_STATE_200MHZ); > + if (IS_ERR(imx_data->pins_100mhz) || > + IS_ERR(imx_data->pins_200mhz)) { > + dev_warn(mmc_dev(host->mmc), > + "could not get ultra high speed state, work on normal mode\n"); > + /* fall back to not support uhs by specify no 1.8v quirk */ > + host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; > + } > + } else { > + host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; > + } > + > err = sdhci_add_host(host); > if (err) > goto disable_clk; > diff --git a/include/linux/platform_data/mmc-esdhc-imx.h b/include/linux/platform_data/mmc-esdhc-imx.h > index d44912d..a0f5a8f 100644 > --- a/include/linux/platform_data/mmc-esdhc-imx.h > +++ b/include/linux/platform_data/mmc-esdhc-imx.h > @@ -10,6 +10,8 @@ > #ifndef __ASM_ARCH_IMX_ESDHC_H > #define __ASM_ARCH_IMX_ESDHC_H > > +#include <linux/types.h> > + > enum wp_types { > ESDHC_WP_NONE, /* no WP, neither controller nor gpio */ > ESDHC_WP_CONTROLLER, /* mmc controller internal WP */ > @@ -32,6 +34,7 @@ enum cd_types { > * @cd_gpio: gpio for card_detect interrupt > * @wp_type: type of write_protect method (see wp_types enum above) > * @cd_type: type of card_detect method (see cd_types enum above) > + * @support_vsel: indicate it supports 1.8v switching > */ > > struct esdhc_platform_data { > @@ -41,5 +44,6 @@ struct esdhc_platform_data { > enum cd_types cd_type; > int max_bus_width; > unsigned int f_max; > + bool support_vsel; > }; > #endif /* __ASM_ARCH_IMX_ESDHC_H */ > -- > 1.7.1 > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 5, 2013 at 2:34 PM, Shawn Guo <shawn.guo@linaro.org> wrote: > On Wed, Sep 04, 2013 at 08:54:14PM +0800, Dong Aisheng wrote: >> Without proper pinctrl state, the card may not be able to work >> on high speed stablely. e.g. SDR104. >> >> This patch add pinctrl state switch code according to different >> uhs mode include 100mhz sate, 200mhz sate and normal state >> (50Mhz and below). >> >> Signed-off-by: Dong Aisheng <b29396@freescale.com> >> --- >> drivers/mmc/host/sdhci-esdhc-imx.c | 110 ++++++++++++++++++++++++++- >> include/linux/platform_data/mmc-esdhc-imx.h | 4 + >> 2 files changed, 113 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c >> index 36b9f63..3e89863 100644 >> --- a/drivers/mmc/host/sdhci-esdhc-imx.c >> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c >> @@ -49,6 +49,10 @@ >> >> #define ESDHC_TUNING_BLOCK_PATTERN_LEN 64 >> >> +/* pinctrl state */ >> +#define ESDHC_PINCTRL_STATE_100MHZ "state_100mhz" >> +#define ESDHC_PINCTRL_STATE_200MHZ "state_200mhz" >> + >> /* >> * Our interpretation of the SDHCI_HOST_CONTROL register >> */ >> @@ -90,6 +94,10 @@ struct pltfm_imx_data { >> u32 scratchpad; >> enum imx_esdhc_type devtype; >> struct pinctrl *pinctrl; >> + struct pinctrl_state *pins_current; >> + struct pinctrl_state *pins_default; >> + struct pinctrl_state *pins_100mhz; >> + struct pinctrl_state *pins_200mhz; >> struct esdhc_platform_data boarddata; >> struct clk *clk_ipg; >> struct clk *clk_ahb; >> @@ -642,6 +650,75 @@ static int esdhc_executing_tuning(struct sdhci_host *host, u32 opcode) >> return ret; >> } >> >> +static int esdhc_change_pinstate(struct sdhci_host *host, >> + unsigned int uhs) >> +{ >> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >> + struct pltfm_imx_data *imx_data = pltfm_host->priv; >> + struct pinctrl_state *pinctrl; >> + int ret; >> + >> + dev_dbg(mmc_dev(host->mmc), "change pinctrl state for uhs %d\n", uhs); >> + >> + if (IS_ERR(imx_data->pinctrl) || >> + IS_ERR(imx_data->pins_default) || >> + IS_ERR(imx_data->pins_100mhz) || >> + IS_ERR(imx_data->pins_200mhz)) >> + return -EINVAL; >> + >> + switch (uhs) { >> + case MMC_TIMING_UHS_SDR12: >> + case MMC_TIMING_UHS_SDR25: >> + case MMC_TIMING_UHS_DDR50: >> + pinctrl = imx_data->pins_default; >> + break; >> + case MMC_TIMING_UHS_SDR50: >> + pinctrl = imx_data->pins_100mhz; >> + break; >> + case MMC_TIMING_UHS_SDR104: >> + pinctrl = imx_data->pins_200mhz; >> + break; >> + default: >> + /* back to default state for other legacy timing */ >> + pinctrl = imx_data->pins_default; >> + } > > So it can be equally written as below? > > switch (uhs) { > case MMC_TIMING_UHS_SDR104: > pinctrl = imx_data->pins_200mhz; > break; > case MMC_TIMING_UHS_SDR50: > pinctrl = imx_data->pins_100mhz; > break; > default: > pinctrl = imx_data->pins_default; > } > Seems good. >> + >> + if (pinctrl == imx_data->pins_current) >> + return 0; > > I think pinctrl_select_state() already take care of the check? > Looks right. I will remove it. >> + >> + ret = pinctrl_select_state(imx_data->pinctrl, pinctrl); >> + if (!ret) >> + imx_data->pins_current = pinctrl; >> + >> + return ret; >> +} >> + >> +static int esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) >> +{ >> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >> + struct pltfm_imx_data *imx_data = pltfm_host->priv; >> + >> + switch (uhs) { >> + case MMC_TIMING_UHS_SDR12: >> + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR12; >> + break; >> + case MMC_TIMING_UHS_SDR25: >> + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR25; >> + break; >> + case MMC_TIMING_UHS_SDR50: >> + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR50; >> + break; >> + case MMC_TIMING_UHS_SDR104: >> + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR104; >> + break; >> + case MMC_TIMING_UHS_DDR50: >> + imx_data->uhs_mode = SDHCI_CTRL_UHS_DDR50; >> + break; >> + } >> + >> + return esdhc_change_pinstate(host, uhs); >> +} >> + >> static const struct sdhci_ops sdhci_esdhc_ops = { >> .read_l = esdhc_readl_le, >> .read_w = esdhc_readw_le, >> @@ -653,6 +730,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = { >> .get_min_clock = esdhc_pltfm_get_min_clock, >> .get_ro = esdhc_pltfm_get_ro, >> .platform_bus_width = esdhc_pltfm_bus_width, >> + .set_uhs_signaling = esdhc_set_uhs_signaling, >> .platform_execute_tuning = esdhc_executing_tuning, >> }; >> >> @@ -695,6 +773,11 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev, >> >> of_property_read_u32(np, "max-frequency", &boarddata->f_max); >> >> + if (of_find_property(np, "no-1-8-v", NULL)) >> + boarddata->support_vsel = false; >> + else >> + boarddata->support_vsel = true; > > So you check "no-1-8-v" property, set support_vsel here and then set > SDHCI_QUIRK2_NO_1_8_V accordingly below in sdhci_esdhc_imx_probe(). > But sdhci_get_of_property() - sdhci-pltfm.c already does the job. > We're not using sdhci_get_of_property. I will consider to use it, that can be another patch later. Regards Dong Aisheng > Shawn > >> + >> return 0; >> } >> #else >> @@ -757,12 +840,20 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) >> clk_prepare_enable(imx_data->clk_ipg); >> clk_prepare_enable(imx_data->clk_ahb); >> >> - imx_data->pinctrl = devm_pinctrl_get_select_default(&pdev->dev); >> + imx_data->pinctrl = devm_pinctrl_get(&pdev->dev); >> if (IS_ERR(imx_data->pinctrl)) { >> err = PTR_ERR(imx_data->pinctrl); >> goto disable_clk; >> } >> >> + imx_data->pins_default = pinctrl_lookup_state(imx_data->pinctrl, >> + PINCTRL_STATE_DEFAULT); >> + if (IS_ERR(imx_data->pins_default)) { >> + err = PTR_ERR(imx_data->pins_default); >> + dev_err(mmc_dev(host->mmc), "could not get default state\n"); >> + goto disable_clk; >> + } >> + >> host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL; >> >> if (is_imx25_esdhc(imx_data) || is_imx35_esdhc(imx_data)) >> @@ -839,6 +930,23 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) >> break; >> } >> >> + /* sdr50 and sdr104 needs work on 1.8v signal voltage */ >> + if ((boarddata->support_vsel) && is_imx6q_usdhc(imx_data)) { >> + imx_data->pins_100mhz = pinctrl_lookup_state(imx_data->pinctrl, >> + ESDHC_PINCTRL_STATE_100MHZ); >> + imx_data->pins_200mhz = pinctrl_lookup_state(imx_data->pinctrl, >> + ESDHC_PINCTRL_STATE_200MHZ); >> + if (IS_ERR(imx_data->pins_100mhz) || >> + IS_ERR(imx_data->pins_200mhz)) { >> + dev_warn(mmc_dev(host->mmc), >> + "could not get ultra high speed state, work on normal mode\n"); >> + /* fall back to not support uhs by specify no 1.8v quirk */ >> + host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; >> + } >> + } else { >> + host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; >> + } >> + >> err = sdhci_add_host(host); >> if (err) >> goto disable_clk; >> diff --git a/include/linux/platform_data/mmc-esdhc-imx.h b/include/linux/platform_data/mmc-esdhc-imx.h >> index d44912d..a0f5a8f 100644 >> --- a/include/linux/platform_data/mmc-esdhc-imx.h >> +++ b/include/linux/platform_data/mmc-esdhc-imx.h >> @@ -10,6 +10,8 @@ >> #ifndef __ASM_ARCH_IMX_ESDHC_H >> #define __ASM_ARCH_IMX_ESDHC_H >> >> +#include <linux/types.h> >> + >> enum wp_types { >> ESDHC_WP_NONE, /* no WP, neither controller nor gpio */ >> ESDHC_WP_CONTROLLER, /* mmc controller internal WP */ >> @@ -32,6 +34,7 @@ enum cd_types { >> * @cd_gpio: gpio for card_detect interrupt >> * @wp_type: type of write_protect method (see wp_types enum above) >> * @cd_type: type of card_detect method (see cd_types enum above) >> + * @support_vsel: indicate it supports 1.8v switching >> */ >> >> struct esdhc_platform_data { >> @@ -41,5 +44,6 @@ struct esdhc_platform_data { >> enum cd_types cd_type; >> int max_bus_width; >> unsigned int f_max; >> + bool support_vsel; >> }; >> #endif /* __ASM_ARCH_IMX_ESDHC_H */ >> -- >> 1.7.1 >> >> > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ulf, On Thu, Sep 5, 2013 at 3:38 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 4 September 2013 14:54, Dong Aisheng <b29396@freescale.com> wrote: >> Without proper pinctrl state, the card may not be able to work >> on high speed stablely. e.g. SDR104. >> >> This patch add pinctrl state switch code according to different >> uhs mode include 100mhz sate, 200mhz sate and normal state >> (50Mhz and below). >> >> Signed-off-by: Dong Aisheng <b29396@freescale.com> >> --- >> drivers/mmc/host/sdhci-esdhc-imx.c | 110 ++++++++++++++++++++++++++- >> include/linux/platform_data/mmc-esdhc-imx.h | 4 + >> 2 files changed, 113 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c >> index 36b9f63..3e89863 100644 >> --- a/drivers/mmc/host/sdhci-esdhc-imx.c >> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c >> @@ -49,6 +49,10 @@ >> >> #define ESDHC_TUNING_BLOCK_PATTERN_LEN 64 >> >> +/* pinctrl state */ >> +#define ESDHC_PINCTRL_STATE_100MHZ "state_100mhz" >> +#define ESDHC_PINCTRL_STATE_200MHZ "state_200mhz" >> + >> /* >> * Our interpretation of the SDHCI_HOST_CONTROL register >> */ >> @@ -90,6 +94,10 @@ struct pltfm_imx_data { >> u32 scratchpad; >> enum imx_esdhc_type devtype; >> struct pinctrl *pinctrl; >> + struct pinctrl_state *pins_current; >> + struct pinctrl_state *pins_default; >> + struct pinctrl_state *pins_100mhz; >> + struct pinctrl_state *pins_200mhz; >> struct esdhc_platform_data boarddata; >> struct clk *clk_ipg; >> struct clk *clk_ahb; >> @@ -642,6 +650,75 @@ static int esdhc_executing_tuning(struct sdhci_host *host, u32 opcode) >> return ret; >> } >> >> +static int esdhc_change_pinstate(struct sdhci_host *host, >> + unsigned int uhs) >> +{ >> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >> + struct pltfm_imx_data *imx_data = pltfm_host->priv; >> + struct pinctrl_state *pinctrl; >> + int ret; >> + >> + dev_dbg(mmc_dev(host->mmc), "change pinctrl state for uhs %d\n", uhs); >> + >> + if (IS_ERR(imx_data->pinctrl) || >> + IS_ERR(imx_data->pins_default) || >> + IS_ERR(imx_data->pins_100mhz) || >> + IS_ERR(imx_data->pins_200mhz)) >> + return -EINVAL; >> + >> + switch (uhs) { >> + case MMC_TIMING_UHS_SDR12: >> + case MMC_TIMING_UHS_SDR25: >> + case MMC_TIMING_UHS_DDR50: >> + pinctrl = imx_data->pins_default; >> + break; >> + case MMC_TIMING_UHS_SDR50: >> + pinctrl = imx_data->pins_100mhz; >> + break; >> + case MMC_TIMING_UHS_SDR104: >> + pinctrl = imx_data->pins_200mhz; >> + break; >> + default: >> + /* back to default state for other legacy timing */ >> + pinctrl = imx_data->pins_default; >> + } >> + >> + if (pinctrl == imx_data->pins_current) >> + return 0; >> + >> + ret = pinctrl_select_state(imx_data->pinctrl, pinctrl); >> + if (!ret) >> + imx_data->pins_current = pinctrl; >> + >> + return ret; >> +} >> + >> +static int esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) >> +{ >> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >> + struct pltfm_imx_data *imx_data = pltfm_host->priv; >> + >> + switch (uhs) { >> + case MMC_TIMING_UHS_SDR12: >> + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR12; >> + break; >> + case MMC_TIMING_UHS_SDR25: >> + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR25; >> + break; >> + case MMC_TIMING_UHS_SDR50: >> + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR50; >> + break; >> + case MMC_TIMING_UHS_SDR104: >> + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR104; >> + break; >> + case MMC_TIMING_UHS_DDR50: >> + imx_data->uhs_mode = SDHCI_CTRL_UHS_DDR50; >> + break; >> + } >> + >> + return esdhc_change_pinstate(host, uhs); >> +} >> + >> static const struct sdhci_ops sdhci_esdhc_ops = { >> .read_l = esdhc_readl_le, >> .read_w = esdhc_readw_le, >> @@ -653,6 +730,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = { >> .get_min_clock = esdhc_pltfm_get_min_clock, >> .get_ro = esdhc_pltfm_get_ro, >> .platform_bus_width = esdhc_pltfm_bus_width, >> + .set_uhs_signaling = esdhc_set_uhs_signaling, >> .platform_execute_tuning = esdhc_executing_tuning, >> }; >> >> @@ -695,6 +773,11 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev, >> >> of_property_read_u32(np, "max-frequency", &boarddata->f_max); >> >> + if (of_find_property(np, "no-1-8-v", NULL)) >> + boarddata->support_vsel = false; >> + else >> + boarddata->support_vsel = true; >> + >> return 0; >> } >> #else >> @@ -757,12 +840,20 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) >> clk_prepare_enable(imx_data->clk_ipg); >> clk_prepare_enable(imx_data->clk_ahb); >> >> - imx_data->pinctrl = devm_pinctrl_get_select_default(&pdev->dev); >> + imx_data->pinctrl = devm_pinctrl_get(&pdev->dev); >> if (IS_ERR(imx_data->pinctrl)) { >> err = PTR_ERR(imx_data->pinctrl); >> goto disable_clk; >> } >> >> + imx_data->pins_default = pinctrl_lookup_state(imx_data->pinctrl, >> + PINCTRL_STATE_DEFAULT); > > I believe you should look into the new pinctrl APIs and the new > sequence at device driver core probe, which automatically fetches > default, idle and sleep state pins. Additionally it sets the default > state. > > It should likely simplifies some code in this patch. > After looking into the pinctrl automatically fetch states, it seems above two line may could be replaced as: imx_data->pinctrl = (&pdev->dev)->pins->p; imx_data->pins_default = (&pdev->dev)->pins->default_state; However, i'm not sure touching the pinctrl internal implementations in device core is a good choice. So i may still like to keep the exist using. Regards Dong Aisheng > Kind regards > Ulf Hansson > > >> + if (IS_ERR(imx_data->pins_default)) { >> + err = PTR_ERR(imx_data->pins_default); >> + dev_err(mmc_dev(host->mmc), "could not get default state\n"); >> + goto disable_clk; >> + } >> + >> host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL; >> >> if (is_imx25_esdhc(imx_data) || is_imx35_esdhc(imx_data)) >> @@ -839,6 +930,23 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) >> break; >> } >> >> + /* sdr50 and sdr104 needs work on 1.8v signal voltage */ >> + if ((boarddata->support_vsel) && is_imx6q_usdhc(imx_data)) { >> + imx_data->pins_100mhz = pinctrl_lookup_state(imx_data->pinctrl, >> + ESDHC_PINCTRL_STATE_100MHZ); >> + imx_data->pins_200mhz = pinctrl_lookup_state(imx_data->pinctrl, >> + ESDHC_PINCTRL_STATE_200MHZ); >> + if (IS_ERR(imx_data->pins_100mhz) || >> + IS_ERR(imx_data->pins_200mhz)) { >> + dev_warn(mmc_dev(host->mmc), >> + "could not get ultra high speed state, work on normal mode\n"); >> + /* fall back to not support uhs by specify no 1.8v quirk */ >> + host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; >> + } >> + } else { >> + host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; >> + } >> + >> err = sdhci_add_host(host); >> if (err) >> goto disable_clk; >> diff --git a/include/linux/platform_data/mmc-esdhc-imx.h b/include/linux/platform_data/mmc-esdhc-imx.h >> index d44912d..a0f5a8f 100644 >> --- a/include/linux/platform_data/mmc-esdhc-imx.h >> +++ b/include/linux/platform_data/mmc-esdhc-imx.h >> @@ -10,6 +10,8 @@ >> #ifndef __ASM_ARCH_IMX_ESDHC_H >> #define __ASM_ARCH_IMX_ESDHC_H >> >> +#include <linux/types.h> >> + >> enum wp_types { >> ESDHC_WP_NONE, /* no WP, neither controller nor gpio */ >> ESDHC_WP_CONTROLLER, /* mmc controller internal WP */ >> @@ -32,6 +34,7 @@ enum cd_types { >> * @cd_gpio: gpio for card_detect interrupt >> * @wp_type: type of write_protect method (see wp_types enum above) >> * @cd_type: type of card_detect method (see cd_types enum above) >> + * @support_vsel: indicate it supports 1.8v switching >> */ >> >> struct esdhc_platform_data { >> @@ -41,5 +44,6 @@ struct esdhc_platform_data { >> enum cd_types cd_type; >> int max_bus_width; >> unsigned int f_max; >> + bool support_vsel; >> }; >> #endif /* __ASM_ARCH_IMX_ESDHC_H */ >> -- >> 1.7.1 >> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> + if (of_find_property(np, "no-1-8-v", NULL)) > + boarddata->support_vsel = false; > + else > + boarddata->support_vsel = true; > + > return 0; No, no, no, no, no :( Please do not just strip the prefix from a definition in code, or just convert the underscores into hyphens to make a device tree property. SDHCI_QUIRK2_NO_1_8_V should never, ever, EVER become no-1-8-v and NO DEFINITION FROM SOURCE CODE should EVER make it into a device tree. This is how we ended up with manure like "dr_mode" in the USB bindings. Please follow good common sense as well as the bindings. In this case a suitable standard for describing a voltage would be no-1v8 (replacing the decimal point with the v) which is how 99.9% of schematics are written in the world where a period is not a desirable character in a net name or component/signal description. But even that is stupid and not expandable. This property should probably be called "voltage-selects" or something and you should put in a list of supported voltages (in uV units just like cpufreq operating points and regulators, please, for consistency, and update the binding). Then you can parse the list and find out precisely what voltages you can switch to - there are more possibilities than an implied 3-ish volts and a switch to 1.8V for MMC. The standard for the OCR register is a bunch of ranges - so specify which voltages you HAVE available, if you can switch. The driver (as per standard) can determine if this is in one range or another. Lack of this property would imply that whatever the chip is driving right now for logic is what it should stay at (support_vsel = false in the code). -- Matt Sealey <neko@bakuhatsu.net> -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Matt, Sorry for the late reply. On Fri, Sep 06, 2013 at 02:40:06AM +0800, Matt Sealey wrote: > > + if (of_find_property(np, "no-1-8-v", NULL)) > > + boarddata->support_vsel = false; > > + else > > + boarddata->support_vsel = true; > > + > > return 0; > > No, no, no, no, no :( > > Please do not just strip the prefix from a definition in code, or just > convert the underscores into hyphens to make a device tree property. > SDHCI_QUIRK2_NO_1_8_V should never, ever, EVER become no-1-8-v and NO > DEFINITION FROM SOURCE CODE should EVER make it into a device tree. > This is how we ended up with manure like "dr_mode" in the USB > bindings. Please follow good common sense as well as the bindings. > > In this case a suitable standard for describing a voltage would be > no-1v8 (replacing the decimal point with the v) which is how 99.9% of > schematics are written in the world where a period is not a desirable > character in a net name or component/signal description. But even that > is stupid and not expandable. > I somehow agree with your idea on the naming. But currently i'm just using the standard property defined in: Documentation/devicetree/bindings/mmc/mmc.txt - no-1-8-v: when present, denotes that 1.8v card voltage is not supported on this system, even if the controller claims it is. > This property should probably be called "voltage-selects" or something > and you should put in a list of supported voltages (in uV units just > like cpufreq operating points and regulators, please, for consistency, > and update the binding). > > Then you can parse the list and find out precisely what voltages you > can switch to - there are more possibilities than an implied 3-ish > volts and a switch to 1.8V for MMC. The standard for the OCR register > is a bunch of ranges - so specify which voltages you HAVE available, > if you can switch. The driver (as per standard) can determine if this > is in one range or another. > > Lack of this property would imply that whatever the chip is driving > right now for logic is what it should stay at (support_vsel = false in > the code). > Here the no-1-8-v is for indicating no 1.8v signal voltage switch support, not the host supply voltage. The spec only defines 1.8v/3.3v switch, so no-1-8-v seems ok for me currently. And for host supply voltage, it's usually provided by host controller and can be retrieved from host capability register. And even if it's using an external regulator, this capability can still be retrieved from the standard regulator API regulator_is_supported_voltage. That's how sdhci driver used currently. So i'm not sure if we really need add the voltage-selects on device tree for mmc since that's regulator's work. Anyway, above is just my initial thoughts on your question. Since this issue actually is not related to this series, maybe you could create a new mail thread about this if you want or patching is welcome for the further discussion more specific. Regards Dong Aisheng > -- > Matt Sealey <neko@bakuhatsu.net> -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5 September 2013 18:04, Dong Aisheng <dongas86@gmail.com> wrote: > Hi Ulf, > > On Thu, Sep 5, 2013 at 3:38 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> On 4 September 2013 14:54, Dong Aisheng <b29396@freescale.com> wrote: >>> Without proper pinctrl state, the card may not be able to work >>> on high speed stablely. e.g. SDR104. >>> >>> This patch add pinctrl state switch code according to different >>> uhs mode include 100mhz sate, 200mhz sate and normal state >>> (50Mhz and below). >>> >>> Signed-off-by: Dong Aisheng <b29396@freescale.com> >>> --- >>> drivers/mmc/host/sdhci-esdhc-imx.c | 110 ++++++++++++++++++++++++++- >>> include/linux/platform_data/mmc-esdhc-imx.h | 4 + >>> 2 files changed, 113 insertions(+), 1 deletions(-) >>> >>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c >>> index 36b9f63..3e89863 100644 >>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c >>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c >>> @@ -49,6 +49,10 @@ >>> >>> #define ESDHC_TUNING_BLOCK_PATTERN_LEN 64 >>> >>> +/* pinctrl state */ >>> +#define ESDHC_PINCTRL_STATE_100MHZ "state_100mhz" >>> +#define ESDHC_PINCTRL_STATE_200MHZ "state_200mhz" >>> + >>> /* >>> * Our interpretation of the SDHCI_HOST_CONTROL register >>> */ >>> @@ -90,6 +94,10 @@ struct pltfm_imx_data { >>> u32 scratchpad; >>> enum imx_esdhc_type devtype; >>> struct pinctrl *pinctrl; >>> + struct pinctrl_state *pins_current; >>> + struct pinctrl_state *pins_default; >>> + struct pinctrl_state *pins_100mhz; >>> + struct pinctrl_state *pins_200mhz; >>> struct esdhc_platform_data boarddata; >>> struct clk *clk_ipg; >>> struct clk *clk_ahb; >>> @@ -642,6 +650,75 @@ static int esdhc_executing_tuning(struct sdhci_host *host, u32 opcode) >>> return ret; >>> } >>> >>> +static int esdhc_change_pinstate(struct sdhci_host *host, >>> + unsigned int uhs) >>> +{ >>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>> + struct pltfm_imx_data *imx_data = pltfm_host->priv; >>> + struct pinctrl_state *pinctrl; >>> + int ret; >>> + >>> + dev_dbg(mmc_dev(host->mmc), "change pinctrl state for uhs %d\n", uhs); >>> + >>> + if (IS_ERR(imx_data->pinctrl) || >>> + IS_ERR(imx_data->pins_default) || >>> + IS_ERR(imx_data->pins_100mhz) || >>> + IS_ERR(imx_data->pins_200mhz)) >>> + return -EINVAL; >>> + >>> + switch (uhs) { >>> + case MMC_TIMING_UHS_SDR12: >>> + case MMC_TIMING_UHS_SDR25: >>> + case MMC_TIMING_UHS_DDR50: >>> + pinctrl = imx_data->pins_default; >>> + break; >>> + case MMC_TIMING_UHS_SDR50: >>> + pinctrl = imx_data->pins_100mhz; >>> + break; >>> + case MMC_TIMING_UHS_SDR104: >>> + pinctrl = imx_data->pins_200mhz; >>> + break; >>> + default: >>> + /* back to default state for other legacy timing */ >>> + pinctrl = imx_data->pins_default; >>> + } >>> + >>> + if (pinctrl == imx_data->pins_current) >>> + return 0; >>> + >>> + ret = pinctrl_select_state(imx_data->pinctrl, pinctrl); >>> + if (!ret) >>> + imx_data->pins_current = pinctrl; >>> + >>> + return ret; >>> +} >>> + >>> +static int esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) >>> +{ >>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>> + struct pltfm_imx_data *imx_data = pltfm_host->priv; >>> + >>> + switch (uhs) { >>> + case MMC_TIMING_UHS_SDR12: >>> + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR12; >>> + break; >>> + case MMC_TIMING_UHS_SDR25: >>> + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR25; >>> + break; >>> + case MMC_TIMING_UHS_SDR50: >>> + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR50; >>> + break; >>> + case MMC_TIMING_UHS_SDR104: >>> + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR104; >>> + break; >>> + case MMC_TIMING_UHS_DDR50: >>> + imx_data->uhs_mode = SDHCI_CTRL_UHS_DDR50; >>> + break; >>> + } >>> + >>> + return esdhc_change_pinstate(host, uhs); >>> +} >>> + >>> static const struct sdhci_ops sdhci_esdhc_ops = { >>> .read_l = esdhc_readl_le, >>> .read_w = esdhc_readw_le, >>> @@ -653,6 +730,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = { >>> .get_min_clock = esdhc_pltfm_get_min_clock, >>> .get_ro = esdhc_pltfm_get_ro, >>> .platform_bus_width = esdhc_pltfm_bus_width, >>> + .set_uhs_signaling = esdhc_set_uhs_signaling, >>> .platform_execute_tuning = esdhc_executing_tuning, >>> }; >>> >>> @@ -695,6 +773,11 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev, >>> >>> of_property_read_u32(np, "max-frequency", &boarddata->f_max); >>> >>> + if (of_find_property(np, "no-1-8-v", NULL)) >>> + boarddata->support_vsel = false; >>> + else >>> + boarddata->support_vsel = true; >>> + >>> return 0; >>> } >>> #else >>> @@ -757,12 +840,20 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) >>> clk_prepare_enable(imx_data->clk_ipg); >>> clk_prepare_enable(imx_data->clk_ahb); >>> >>> - imx_data->pinctrl = devm_pinctrl_get_select_default(&pdev->dev); >>> + imx_data->pinctrl = devm_pinctrl_get(&pdev->dev); >>> if (IS_ERR(imx_data->pinctrl)) { >>> err = PTR_ERR(imx_data->pinctrl); >>> goto disable_clk; >>> } >>> >>> + imx_data->pins_default = pinctrl_lookup_state(imx_data->pinctrl, >>> + PINCTRL_STATE_DEFAULT); >> >> I believe you should look into the new pinctrl APIs and the new >> sequence at device driver core probe, which automatically fetches >> default, idle and sleep state pins. Additionally it sets the default >> state. >> >> It should likely simplifies some code in this patch. >> > > After looking into the pinctrl automatically fetch states, it seems > above two line may could be replaced as: > imx_data->pinctrl = (&pdev->dev)->pins->p; > imx_data->pins_default = (&pdev->dev)->pins->default_state; pinctrl_pm_select_default_state(dev) pinctrl_pm_select_ilde_state(dev) pinctrl_pm_select_sleep_state(dev) The states are fetched from device core at probe. Kind regards Ulf Hansson > > However, i'm not sure touching the pinctrl internal implementations in > device core is a good choice. > So i may still like to keep the exist using. > > Regards > Dong Aisheng > >> Kind regards >> Ulf Hansson >> >> >>> + if (IS_ERR(imx_data->pins_default)) { >>> + err = PTR_ERR(imx_data->pins_default); >>> + dev_err(mmc_dev(host->mmc), "could not get default state\n"); >>> + goto disable_clk; >>> + } >>> + >>> host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL; >>> >>> if (is_imx25_esdhc(imx_data) || is_imx35_esdhc(imx_data)) >>> @@ -839,6 +930,23 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) >>> break; >>> } >>> >>> + /* sdr50 and sdr104 needs work on 1.8v signal voltage */ >>> + if ((boarddata->support_vsel) && is_imx6q_usdhc(imx_data)) { >>> + imx_data->pins_100mhz = pinctrl_lookup_state(imx_data->pinctrl, >>> + ESDHC_PINCTRL_STATE_100MHZ); >>> + imx_data->pins_200mhz = pinctrl_lookup_state(imx_data->pinctrl, >>> + ESDHC_PINCTRL_STATE_200MHZ); >>> + if (IS_ERR(imx_data->pins_100mhz) || >>> + IS_ERR(imx_data->pins_200mhz)) { >>> + dev_warn(mmc_dev(host->mmc), >>> + "could not get ultra high speed state, work on normal mode\n"); >>> + /* fall back to not support uhs by specify no 1.8v quirk */ >>> + host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; >>> + } >>> + } else { >>> + host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; >>> + } >>> + >>> err = sdhci_add_host(host); >>> if (err) >>> goto disable_clk; >>> diff --git a/include/linux/platform_data/mmc-esdhc-imx.h b/include/linux/platform_data/mmc-esdhc-imx.h >>> index d44912d..a0f5a8f 100644 >>> --- a/include/linux/platform_data/mmc-esdhc-imx.h >>> +++ b/include/linux/platform_data/mmc-esdhc-imx.h >>> @@ -10,6 +10,8 @@ >>> #ifndef __ASM_ARCH_IMX_ESDHC_H >>> #define __ASM_ARCH_IMX_ESDHC_H >>> >>> +#include <linux/types.h> >>> + >>> enum wp_types { >>> ESDHC_WP_NONE, /* no WP, neither controller nor gpio */ >>> ESDHC_WP_CONTROLLER, /* mmc controller internal WP */ >>> @@ -32,6 +34,7 @@ enum cd_types { >>> * @cd_gpio: gpio for card_detect interrupt >>> * @wp_type: type of write_protect method (see wp_types enum above) >>> * @cd_type: type of card_detect method (see cd_types enum above) >>> + * @support_vsel: indicate it supports 1.8v switching >>> */ >>> >>> struct esdhc_platform_data { >>> @@ -41,5 +44,6 @@ struct esdhc_platform_data { >>> enum cd_types cd_type; >>> int max_bus_width; >>> unsigned int f_max; >>> + bool support_vsel; >>> }; >>> #endif /* __ASM_ARCH_IMX_ESDHC_H */ >>> -- >>> 1.7.1 >>> >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ulf, Thanks for the reply. On Fri, Sep 13, 2013 at 10:01 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 5 September 2013 18:04, Dong Aisheng <dongas86@gmail.com> wrote: >> Hi Ulf, >> >> On Thu, Sep 5, 2013 at 3:38 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>> On 4 September 2013 14:54, Dong Aisheng <b29396@freescale.com> wrote: >>>> Without proper pinctrl state, the card may not be able to work >>>> on high speed stablely. e.g. SDR104. >>>> >>>> This patch add pinctrl state switch code according to different >>>> uhs mode include 100mhz sate, 200mhz sate and normal state >>>> (50Mhz and below). >>>> >>>> Signed-off-by: Dong Aisheng <b29396@freescale.com> >>>> --- >>>> drivers/mmc/host/sdhci-esdhc-imx.c | 110 ++++++++++++++++++++++++++- >>>> include/linux/platform_data/mmc-esdhc-imx.h | 4 + >>>> 2 files changed, 113 insertions(+), 1 deletions(-) >>>> >>>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c >>>> index 36b9f63..3e89863 100644 >>>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c >>>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c >>>> @@ -49,6 +49,10 @@ >>>> >>>> #define ESDHC_TUNING_BLOCK_PATTERN_LEN 64 >>>> >>>> +/* pinctrl state */ >>>> +#define ESDHC_PINCTRL_STATE_100MHZ "state_100mhz" >>>> +#define ESDHC_PINCTRL_STATE_200MHZ "state_200mhz" >>>> + >>>> /* >>>> * Our interpretation of the SDHCI_HOST_CONTROL register >>>> */ >>>> @@ -90,6 +94,10 @@ struct pltfm_imx_data { >>>> u32 scratchpad; >>>> enum imx_esdhc_type devtype; >>>> struct pinctrl *pinctrl; >>>> + struct pinctrl_state *pins_current; >>>> + struct pinctrl_state *pins_default; >>>> + struct pinctrl_state *pins_100mhz; >>>> + struct pinctrl_state *pins_200mhz; >>>> struct esdhc_platform_data boarddata; >>>> struct clk *clk_ipg; >>>> struct clk *clk_ahb; >>>> @@ -642,6 +650,75 @@ static int esdhc_executing_tuning(struct sdhci_host *host, u32 opcode) >>>> return ret; >>>> } >>>> >>>> +static int esdhc_change_pinstate(struct sdhci_host *host, >>>> + unsigned int uhs) >>>> +{ >>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>> + struct pltfm_imx_data *imx_data = pltfm_host->priv; >>>> + struct pinctrl_state *pinctrl; >>>> + int ret; >>>> + >>>> + dev_dbg(mmc_dev(host->mmc), "change pinctrl state for uhs %d\n", uhs); >>>> + >>>> + if (IS_ERR(imx_data->pinctrl) || >>>> + IS_ERR(imx_data->pins_default) || >>>> + IS_ERR(imx_data->pins_100mhz) || >>>> + IS_ERR(imx_data->pins_200mhz)) >>>> + return -EINVAL; >>>> + >>>> + switch (uhs) { >>>> + case MMC_TIMING_UHS_SDR12: >>>> + case MMC_TIMING_UHS_SDR25: >>>> + case MMC_TIMING_UHS_DDR50: >>>> + pinctrl = imx_data->pins_default; >>>> + break; >>>> + case MMC_TIMING_UHS_SDR50: >>>> + pinctrl = imx_data->pins_100mhz; >>>> + break; >>>> + case MMC_TIMING_UHS_SDR104: >>>> + pinctrl = imx_data->pins_200mhz; >>>> + break; >>>> + default: >>>> + /* back to default state for other legacy timing */ >>>> + pinctrl = imx_data->pins_default; >>>> + } >>>> + >>>> + if (pinctrl == imx_data->pins_current) >>>> + return 0; >>>> + >>>> + ret = pinctrl_select_state(imx_data->pinctrl, pinctrl); >>>> + if (!ret) >>>> + imx_data->pins_current = pinctrl; >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) >>>> +{ >>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>> + struct pltfm_imx_data *imx_data = pltfm_host->priv; >>>> + >>>> + switch (uhs) { >>>> + case MMC_TIMING_UHS_SDR12: >>>> + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR12; >>>> + break; >>>> + case MMC_TIMING_UHS_SDR25: >>>> + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR25; >>>> + break; >>>> + case MMC_TIMING_UHS_SDR50: >>>> + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR50; >>>> + break; >>>> + case MMC_TIMING_UHS_SDR104: >>>> + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR104; >>>> + break; >>>> + case MMC_TIMING_UHS_DDR50: >>>> + imx_data->uhs_mode = SDHCI_CTRL_UHS_DDR50; >>>> + break; >>>> + } >>>> + >>>> + return esdhc_change_pinstate(host, uhs); >>>> +} >>>> + >>>> static const struct sdhci_ops sdhci_esdhc_ops = { >>>> .read_l = esdhc_readl_le, >>>> .read_w = esdhc_readw_le, >>>> @@ -653,6 +730,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = { >>>> .get_min_clock = esdhc_pltfm_get_min_clock, >>>> .get_ro = esdhc_pltfm_get_ro, >>>> .platform_bus_width = esdhc_pltfm_bus_width, >>>> + .set_uhs_signaling = esdhc_set_uhs_signaling, >>>> .platform_execute_tuning = esdhc_executing_tuning, >>>> }; >>>> >>>> @@ -695,6 +773,11 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev, >>>> >>>> of_property_read_u32(np, "max-frequency", &boarddata->f_max); >>>> >>>> + if (of_find_property(np, "no-1-8-v", NULL)) >>>> + boarddata->support_vsel = false; >>>> + else >>>> + boarddata->support_vsel = true; >>>> + >>>> return 0; >>>> } >>>> #else >>>> @@ -757,12 +840,20 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) >>>> clk_prepare_enable(imx_data->clk_ipg); >>>> clk_prepare_enable(imx_data->clk_ahb); >>>> >>>> - imx_data->pinctrl = devm_pinctrl_get_select_default(&pdev->dev); >>>> + imx_data->pinctrl = devm_pinctrl_get(&pdev->dev); >>>> if (IS_ERR(imx_data->pinctrl)) { >>>> err = PTR_ERR(imx_data->pinctrl); >>>> goto disable_clk; >>>> } >>>> >>>> + imx_data->pins_default = pinctrl_lookup_state(imx_data->pinctrl, >>>> + PINCTRL_STATE_DEFAULT); >>> >>> I believe you should look into the new pinctrl APIs and the new >>> sequence at device driver core probe, which automatically fetches >>> default, idle and sleep state pins. Additionally it sets the default >>> state. >>> >>> It should likely simplifies some code in this patch. >>> >> >> After looking into the pinctrl automatically fetch states, it seems >> above two line may could be replaced as: >> imx_data->pinctrl = (&pdev->dev)->pins->p; >> imx_data->pins_default = (&pdev->dev)->pins->default_state; > > pinctrl_pm_select_default_state(dev) > pinctrl_pm_select_ilde_state(dev) > pinctrl_pm_select_sleep_state(dev) > > The states are fetched from device core at probe. > I did not use idle and sleep state, instead, i need get another two private defined states, state_100mhz, state_200mhz and select between them, so these API seems not so useful for my case. Even i want to use this API, i can only use it for default state which seems not so neccesary. Regards Dong Aisheng > Kind regards > Ulf Hansson > > >> >> However, i'm not sure touching the pinctrl internal implementations in >> device core is a good choice. >> So i may still like to keep the exist using. >> >> Regards >> Dong Aisheng >> >>> Kind regards >>> Ulf Hansson >>> >>> >>>> + if (IS_ERR(imx_data->pins_default)) { >>>> + err = PTR_ERR(imx_data->pins_default); >>>> + dev_err(mmc_dev(host->mmc), "could not get default state\n"); >>>> + goto disable_clk; >>>> + } >>>> + >>>> host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL; >>>> >>>> if (is_imx25_esdhc(imx_data) || is_imx35_esdhc(imx_data)) >>>> @@ -839,6 +930,23 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) >>>> break; >>>> } >>>> >>>> + /* sdr50 and sdr104 needs work on 1.8v signal voltage */ >>>> + if ((boarddata->support_vsel) && is_imx6q_usdhc(imx_data)) { >>>> + imx_data->pins_100mhz = pinctrl_lookup_state(imx_data->pinctrl, >>>> + ESDHC_PINCTRL_STATE_100MHZ); >>>> + imx_data->pins_200mhz = pinctrl_lookup_state(imx_data->pinctrl, >>>> + ESDHC_PINCTRL_STATE_200MHZ); >>>> + if (IS_ERR(imx_data->pins_100mhz) || >>>> + IS_ERR(imx_data->pins_200mhz)) { >>>> + dev_warn(mmc_dev(host->mmc), >>>> + "could not get ultra high speed state, work on normal mode\n"); >>>> + /* fall back to not support uhs by specify no 1.8v quirk */ >>>> + host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; >>>> + } >>>> + } else { >>>> + host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; >>>> + } >>>> + >>>> err = sdhci_add_host(host); >>>> if (err) >>>> goto disable_clk; >>>> diff --git a/include/linux/platform_data/mmc-esdhc-imx.h b/include/linux/platform_data/mmc-esdhc-imx.h >>>> index d44912d..a0f5a8f 100644 >>>> --- a/include/linux/platform_data/mmc-esdhc-imx.h >>>> +++ b/include/linux/platform_data/mmc-esdhc-imx.h >>>> @@ -10,6 +10,8 @@ >>>> #ifndef __ASM_ARCH_IMX_ESDHC_H >>>> #define __ASM_ARCH_IMX_ESDHC_H >>>> >>>> +#include <linux/types.h> >>>> + >>>> enum wp_types { >>>> ESDHC_WP_NONE, /* no WP, neither controller nor gpio */ >>>> ESDHC_WP_CONTROLLER, /* mmc controller internal WP */ >>>> @@ -32,6 +34,7 @@ enum cd_types { >>>> * @cd_gpio: gpio for card_detect interrupt >>>> * @wp_type: type of write_protect method (see wp_types enum above) >>>> * @cd_type: type of card_detect method (see cd_types enum above) >>>> + * @support_vsel: indicate it supports 1.8v switching >>>> */ >>>> >>>> struct esdhc_platform_data { >>>> @@ -41,5 +44,6 @@ struct esdhc_platform_data { >>>> enum cd_types cd_type; >>>> int max_bus_width; >>>> unsigned int f_max; >>>> + bool support_vsel; >>>> }; >>>> #endif /* __ASM_ARCH_IMX_ESDHC_H */ >>>> -- >>>> 1.7.1 >>>> >>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >>> _______________________________________________ >>> linux-arm-kernel mailing list >>> linux-arm-kernel@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 11, 2013 at 4:26 AM, Dong Aisheng <b29396@freescale.com> wrote: > Hi Matt, > > I somehow agree with your idea on the naming. > But currently i'm just using the standard property defined in: > Documentation/devicetree/bindings/mmc/mmc.txt > - no-1-8-v: when present, denotes that 1.8v card voltage is not supported on > this system, even if the controller claims it is. Whoever did that needs kicking :< >> This property should probably be called "voltage-selects" or something >> and you should put in a list of supported voltages (in uV units just >> like cpufreq operating points and regulators, please, for consistency, >> and update the binding). > > Here the no-1-8-v is for indicating no 1.8v signal voltage switch support, > not the host supply voltage. > The spec only defines 1.8v/3.3v switch, so no-1-8-v seems ok for me currently. I thought the spec also has a 1.2V mode.. so what do the MMC guys propose, adding no-1-2-v properties too? And if there exists in the future a 0.8V mode, another property? And another? > That's how sdhci driver used currently. > So i'm not sure if we really need add the voltage-selects on device tree > for mmc since that's regulator's work. Understood. My initial thought is simply that the MMC class should define a behavior which you could assume - in this case, that a host supports a certain set of voltages (for supply and for signalling), and a card supports a certain set of voltages (for supply and for signalling). In the case of the combinations here, this is all in the spec and defined. However, what device trees should do is essentially provide information where the defaults or documentation contain assumptions, and supplant them when necessary. An MMC host controller given a regulator now knows it can change voltages. But to what values? Most regulators are fixed or have a wide range. A regulator that is defined for 1.8V and 3.3V is defined as 1.8V *through* 3.3V meaning you COULD set it to 2.5V if you wanted to. A property defining the valid voltages is good here. For the case where someone screwed a board design or a regulator is weak somehow, you could put 1.85V in the property to say, this is my close-enough-to-1.8V value. Otherwise what happens is you get a regulator set failure if the range didn't include 1.8V, or weird issues because the voltage isn't high enough *for the design*. For signalling voltage, sure, there could be 1.8V and 3.3V defined in the host and the card, but the same applies - assume those in the driver, and the DT can define which ones are valid. For a driver probing, it can look in the DT table and see, I have a matching 1800000uV and a matching 3300000uV voltage. If it sees 1200000uV and it has no clue how to orient the system into using that voltage, it ignores it (it couldn't use it anyway, since it would have no knowledge of how to instruct a card to signal at that voltage). This works for the supply regulator setting and the signalling setting with a common, future-proof format.. In the future, it may work when the kernel is updated to support this as-of-yet non-existing functionality and the DT would have been correct from the start. > Anyway, above is just my initial thoughts on your question. > Since this issue actually is not related to this series, > maybe you could create a new mail thread about this if you want > or patching is welcome for the further discussion more specific. Well, a new thread.. sure. That might be for another day (it took me 2 weeks to get back to this one, I don't know the next time I will have enough time to go through LAKML :) I have written it on my whiteboard for future discussion, so I'll bring it up again as soon as I can..
diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c index 36b9f63..3e89863 100644 --- a/drivers/mmc/host/sdhci-esdhc-imx.c +++ b/drivers/mmc/host/sdhci-esdhc-imx.c @@ -49,6 +49,10 @@ #define ESDHC_TUNING_BLOCK_PATTERN_LEN 64 +/* pinctrl state */ +#define ESDHC_PINCTRL_STATE_100MHZ "state_100mhz" +#define ESDHC_PINCTRL_STATE_200MHZ "state_200mhz" + /* * Our interpretation of the SDHCI_HOST_CONTROL register */ @@ -90,6 +94,10 @@ struct pltfm_imx_data { u32 scratchpad; enum imx_esdhc_type devtype; struct pinctrl *pinctrl; + struct pinctrl_state *pins_current; + struct pinctrl_state *pins_default; + struct pinctrl_state *pins_100mhz; + struct pinctrl_state *pins_200mhz; struct esdhc_platform_data boarddata; struct clk *clk_ipg; struct clk *clk_ahb; @@ -642,6 +650,75 @@ static int esdhc_executing_tuning(struct sdhci_host *host, u32 opcode) return ret; } +static int esdhc_change_pinstate(struct sdhci_host *host, + unsigned int uhs) +{ + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct pltfm_imx_data *imx_data = pltfm_host->priv; + struct pinctrl_state *pinctrl; + int ret; + + dev_dbg(mmc_dev(host->mmc), "change pinctrl state for uhs %d\n", uhs); + + if (IS_ERR(imx_data->pinctrl) || + IS_ERR(imx_data->pins_default) || + IS_ERR(imx_data->pins_100mhz) || + IS_ERR(imx_data->pins_200mhz)) + return -EINVAL; + + switch (uhs) { + case MMC_TIMING_UHS_SDR12: + case MMC_TIMING_UHS_SDR25: + case MMC_TIMING_UHS_DDR50: + pinctrl = imx_data->pins_default; + break; + case MMC_TIMING_UHS_SDR50: + pinctrl = imx_data->pins_100mhz; + break; + case MMC_TIMING_UHS_SDR104: + pinctrl = imx_data->pins_200mhz; + break; + default: + /* back to default state for other legacy timing */ + pinctrl = imx_data->pins_default; + } + + if (pinctrl == imx_data->pins_current) + return 0; + + ret = pinctrl_select_state(imx_data->pinctrl, pinctrl); + if (!ret) + imx_data->pins_current = pinctrl; + + return ret; +} + +static int esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) +{ + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct pltfm_imx_data *imx_data = pltfm_host->priv; + + switch (uhs) { + case MMC_TIMING_UHS_SDR12: + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR12; + break; + case MMC_TIMING_UHS_SDR25: + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR25; + break; + case MMC_TIMING_UHS_SDR50: + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR50; + break; + case MMC_TIMING_UHS_SDR104: + imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR104; + break; + case MMC_TIMING_UHS_DDR50: + imx_data->uhs_mode = SDHCI_CTRL_UHS_DDR50; + break; + } + + return esdhc_change_pinstate(host, uhs); +} + static const struct sdhci_ops sdhci_esdhc_ops = { .read_l = esdhc_readl_le, .read_w = esdhc_readw_le, @@ -653,6 +730,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = { .get_min_clock = esdhc_pltfm_get_min_clock, .get_ro = esdhc_pltfm_get_ro, .platform_bus_width = esdhc_pltfm_bus_width, + .set_uhs_signaling = esdhc_set_uhs_signaling, .platform_execute_tuning = esdhc_executing_tuning, }; @@ -695,6 +773,11 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev, of_property_read_u32(np, "max-frequency", &boarddata->f_max); + if (of_find_property(np, "no-1-8-v", NULL)) + boarddata->support_vsel = false; + else + boarddata->support_vsel = true; + return 0; } #else @@ -757,12 +840,20 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) clk_prepare_enable(imx_data->clk_ipg); clk_prepare_enable(imx_data->clk_ahb); - imx_data->pinctrl = devm_pinctrl_get_select_default(&pdev->dev); + imx_data->pinctrl = devm_pinctrl_get(&pdev->dev); if (IS_ERR(imx_data->pinctrl)) { err = PTR_ERR(imx_data->pinctrl); goto disable_clk; } + imx_data->pins_default = pinctrl_lookup_state(imx_data->pinctrl, + PINCTRL_STATE_DEFAULT); + if (IS_ERR(imx_data->pins_default)) { + err = PTR_ERR(imx_data->pins_default); + dev_err(mmc_dev(host->mmc), "could not get default state\n"); + goto disable_clk; + } + host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL; if (is_imx25_esdhc(imx_data) || is_imx35_esdhc(imx_data)) @@ -839,6 +930,23 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) break; } + /* sdr50 and sdr104 needs work on 1.8v signal voltage */ + if ((boarddata->support_vsel) && is_imx6q_usdhc(imx_data)) { + imx_data->pins_100mhz = pinctrl_lookup_state(imx_data->pinctrl, + ESDHC_PINCTRL_STATE_100MHZ); + imx_data->pins_200mhz = pinctrl_lookup_state(imx_data->pinctrl, + ESDHC_PINCTRL_STATE_200MHZ); + if (IS_ERR(imx_data->pins_100mhz) || + IS_ERR(imx_data->pins_200mhz)) { + dev_warn(mmc_dev(host->mmc), + "could not get ultra high speed state, work on normal mode\n"); + /* fall back to not support uhs by specify no 1.8v quirk */ + host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; + } + } else { + host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; + } + err = sdhci_add_host(host); if (err) goto disable_clk; diff --git a/include/linux/platform_data/mmc-esdhc-imx.h b/include/linux/platform_data/mmc-esdhc-imx.h index d44912d..a0f5a8f 100644 --- a/include/linux/platform_data/mmc-esdhc-imx.h +++ b/include/linux/platform_data/mmc-esdhc-imx.h @@ -10,6 +10,8 @@ #ifndef __ASM_ARCH_IMX_ESDHC_H #define __ASM_ARCH_IMX_ESDHC_H +#include <linux/types.h> + enum wp_types { ESDHC_WP_NONE, /* no WP, neither controller nor gpio */ ESDHC_WP_CONTROLLER, /* mmc controller internal WP */ @@ -32,6 +34,7 @@ enum cd_types { * @cd_gpio: gpio for card_detect interrupt * @wp_type: type of write_protect method (see wp_types enum above) * @cd_type: type of card_detect method (see cd_types enum above) + * @support_vsel: indicate it supports 1.8v switching */ struct esdhc_platform_data { @@ -41,5 +44,6 @@ struct esdhc_platform_data { enum cd_types cd_type; int max_bus_width; unsigned int f_max; + bool support_vsel; }; #endif /* __ASM_ARCH_IMX_ESDHC_H */
Without proper pinctrl state, the card may not be able to work on high speed stablely. e.g. SDR104. This patch add pinctrl state switch code according to different uhs mode include 100mhz sate, 200mhz sate and normal state (50Mhz and below). Signed-off-by: Dong Aisheng <b29396@freescale.com> --- drivers/mmc/host/sdhci-esdhc-imx.c | 110 ++++++++++++++++++++++++++- include/linux/platform_data/mmc-esdhc-imx.h | 4 + 2 files changed, 113 insertions(+), 1 deletions(-)