| Message ID | 1441624721-15612-5-git-send-email-vaibhav.hiremath@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 7 Sep 2015 16:48:38 +0530 Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote: > Different bus clock may need different pin setting. > For example, fast bus clock like 208Mhz need pin drive fast > while slow bus clock prefer pin drive slow to guarantee > signal quality. > > So this patch creates two states, > - Default (slow/normal) pin state > - And fast pin state for higher freq bus speed. > > And selection of pin state is done based on timing mode. > > Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org> > Signed-off-by: Kevin Liu <kliu5@marvell.com> > --- > drivers/mmc/host/sdhci-pxav3.c | 45 +++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 44 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c > index c2b2b78..d933f75 100644 > --- a/drivers/mmc/host/sdhci-pxav3.c > +++ b/drivers/mmc/host/sdhci-pxav3.c > @@ -35,6 +35,7 @@ > #include <linux/pm.h> > #include <linux/pm_runtime.h> > #include <linux/mbus.h> > +#include <linux/pinctrl/consumer.h> > > #include "sdhci.h" > #include "sdhci-pltfm.h" > @@ -92,6 +93,10 @@ struct sdhci_pxa { > void __iomem *io_pwr_reg; > void __iomem *io_pwr_lock_reg; > struct sdhci_pxa_data *data; > + > + struct pinctrl *pinctrl; > + struct pinctrl_state *pins_default; > + struct pinctrl_state *pins_fast; > }; > > static struct sdhci_pxa_data pxav3_data_v1 = { > @@ -298,6 +303,33 @@ static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode) > pxa->power_mode = power_mode; > } > > +static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs) > +{ > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_pxa *pxa = pltfm_host->priv; > + struct pinctrl_state *pinctrl; > + > + if (IS_ERR(pxa->pinctrl) || > + IS_ERR(pxa->pins_default) || > + IS_ERR(pxa->pins_fast)) > + return -EINVAL; > + > + switch (uhs) { > + case MMC_TIMING_UHS_SDR50: > + case MMC_TIMING_UHS_SDR104: > + case MMC_TIMING_MMC_HS200: > + case MMC_TIMING_MMC_HS400: > + pinctrl = pxa->pins_fast; > + break; > + default: > + /* back to default state for other legacy timing */ > + pinctrl = pxa->pins_default; > + break; > + } > + > + return pinctrl_select_state(pxa->pinctrl, pinctrl); > +} > + > static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) > { > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > @@ -353,6 +385,8 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) > dev_dbg(mmc_dev(host->mmc), > "%s uhs = %d, ctrl_2 = %04X\n", > __func__, uhs, ctrl_2); > + > + pxav3_select_pinstate(host, uhs); return pxav3_select_pinstate(host, uhs);? And could we ignore this call for those SDHCI hosts that don't need it? > } > > static void pxav3_voltage_switch(struct sdhci_host *host, > @@ -416,7 +450,6 @@ static void pxav3_set_clock(struct sdhci_host *host, unsigned int clock) > /* TX internal clock selection */ > pxav3_set_tx_clock(host); > } > - why not fix this in patch 3? > } > > static const struct sdhci_ops pxav3_sdhci_ops = { > @@ -586,6 +619,16 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) > } > } > > + pxa->pinctrl = devm_pinctrl_get(dev); could we ignore this for those SDHCI hosts that don't need it? > + if (!IS_ERR(pxa->pinctrl)) { > + pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default"); > + if (IS_ERR(pxa->pins_default)) > + dev_err(dev, "could not get default pinstate\n"); Why those SDHCI hosts that don't need pinctl setting should got this error? > + pxa->pins_fast = pinctrl_lookup_state(pxa->pinctrl, "fast"); > + if (IS_ERR(pxa->pins_fast)) > + dev_info(dev, "could not get fast pinstate\n"); > + } > + > pm_runtime_get_noresume(&pdev->dev); > pm_runtime_set_active(&pdev->dev); > pm_runtime_set_autosuspend_delay(&pdev->dev, PXAV3_RPM_DELAY_MS);
On Mon, 7 Sep 2015 16:48:38 +0530 Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote: > Different bus clock may need different pin setting. > For example, fast bus clock like 208Mhz need pin drive fast > while slow bus clock prefer pin drive slow to guarantee > signal quality. > > So this patch creates two states, > - Default (slow/normal) pin state > - And fast pin state for higher freq bus speed. > > And selection of pin state is done based on timing mode. > > Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org> > Signed-off-by: Kevin Liu <kliu5@marvell.com> > --- > drivers/mmc/host/sdhci-pxav3.c | 45 +++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 44 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c > index c2b2b78..d933f75 100644 > --- a/drivers/mmc/host/sdhci-pxav3.c > +++ b/drivers/mmc/host/sdhci-pxav3.c > @@ -35,6 +35,7 @@ > #include <linux/pm.h> > #include <linux/pm_runtime.h> > #include <linux/mbus.h> > +#include <linux/pinctrl/consumer.h> > > #include "sdhci.h" > #include "sdhci-pltfm.h" > @@ -92,6 +93,10 @@ struct sdhci_pxa { > void __iomem *io_pwr_reg; > void __iomem *io_pwr_lock_reg; > struct sdhci_pxa_data *data; > + > + struct pinctrl *pinctrl; > + struct pinctrl_state *pins_default; > + struct pinctrl_state *pins_fast; > }; > > static struct sdhci_pxa_data pxav3_data_v1 = { > @@ -298,6 +303,33 @@ static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode) > pxa->power_mode = power_mode; > } > > +static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs) > +{ > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_pxa *pxa = pltfm_host->priv; > + struct pinctrl_state *pinctrl; > + > + if (IS_ERR(pxa->pinctrl) || > + IS_ERR(pxa->pins_default) || > + IS_ERR(pxa->pins_fast)) > + return -EINVAL; > + > + switch (uhs) { > + case MMC_TIMING_UHS_SDR50: > + case MMC_TIMING_UHS_SDR104: > + case MMC_TIMING_MMC_HS200: > + case MMC_TIMING_MMC_HS400: Are you sure this IP can support HS400? > + pinctrl = pxa->pins_fast; > + break; > + default: > + /* back to default state for other legacy timing */ > + pinctrl = pxa->pins_default; > + break; > + } > + > + return pinctrl_select_state(pxa->pinctrl, pinctrl); > +} > + > static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) > { > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > @@ -353,6 +385,8 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) > dev_dbg(mmc_dev(host->mmc), > "%s uhs = %d, ctrl_2 = %04X\n", > __func__, uhs, ctrl_2); > + > + pxav3_select_pinstate(host, uhs); > } > > static void pxav3_voltage_switch(struct sdhci_host *host, > @@ -416,7 +450,6 @@ static void pxav3_set_clock(struct sdhci_host *host, unsigned int clock) > /* TX internal clock selection */ > pxav3_set_tx_clock(host); > } > - > } > > static const struct sdhci_ops pxav3_sdhci_ops = { > @@ -586,6 +619,16 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) > } > } > > + pxa->pinctrl = devm_pinctrl_get(dev); > + if (!IS_ERR(pxa->pinctrl)) { > + pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default"); > + if (IS_ERR(pxa->pins_default)) > + dev_err(dev, "could not get default pinstate\n"); > + pxa->pins_fast = pinctrl_lookup_state(pxa->pinctrl, "fast"); > + if (IS_ERR(pxa->pins_fast)) > + dev_info(dev, "could not get fast pinstate\n"); > + } > + > pm_runtime_get_noresume(&pdev->dev); > pm_runtime_set_active(&pdev->dev); > pm_runtime_set_autosuspend_delay(&pdev->dev, PXAV3_RPM_DELAY_MS);
On Tuesday 08 September 2015 12:22 PM, Jisheng Zhang wrote: > On Mon, 7 Sep 2015 16:48:38 +0530 > Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote: > >> Different bus clock may need different pin setting. >> For example, fast bus clock like 208Mhz need pin drive fast >> while slow bus clock prefer pin drive slow to guarantee >> signal quality. >> >> So this patch creates two states, >> - Default (slow/normal) pin state >> - And fast pin state for higher freq bus speed. >> >> And selection of pin state is done based on timing mode. >> >> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org> >> Signed-off-by: Kevin Liu <kliu5@marvell.com> >> --- >> drivers/mmc/host/sdhci-pxav3.c | 45 +++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 44 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c >> index c2b2b78..d933f75 100644 >> --- a/drivers/mmc/host/sdhci-pxav3.c >> +++ b/drivers/mmc/host/sdhci-pxav3.c >> @@ -35,6 +35,7 @@ >> #include <linux/pm.h> >> #include <linux/pm_runtime.h> >> #include <linux/mbus.h> >> +#include <linux/pinctrl/consumer.h> >> >> #include "sdhci.h" >> #include "sdhci-pltfm.h" >> @@ -92,6 +93,10 @@ struct sdhci_pxa { >> void __iomem *io_pwr_reg; >> void __iomem *io_pwr_lock_reg; >> struct sdhci_pxa_data *data; >> + >> + struct pinctrl *pinctrl; >> + struct pinctrl_state *pins_default; >> + struct pinctrl_state *pins_fast; >> }; >> >> static struct sdhci_pxa_data pxav3_data_v1 = { >> @@ -298,6 +303,33 @@ static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode) >> pxa->power_mode = power_mode; >> } >> >> +static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs) >> +{ >> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >> + struct sdhci_pxa *pxa = pltfm_host->priv; >> + struct pinctrl_state *pinctrl; >> + >> + if (IS_ERR(pxa->pinctrl) || >> + IS_ERR(pxa->pins_default) || >> + IS_ERR(pxa->pins_fast)) >> + return -EINVAL; >> + >> + switch (uhs) { >> + case MMC_TIMING_UHS_SDR50: >> + case MMC_TIMING_UHS_SDR104: >> + case MMC_TIMING_MMC_HS200: >> + case MMC_TIMING_MMC_HS400: >> + pinctrl = pxa->pins_fast; >> + break; >> + default: >> + /* back to default state for other legacy timing */ >> + pinctrl = pxa->pins_default; >> + break; >> + } >> + >> + return pinctrl_select_state(pxa->pinctrl, pinctrl); >> +} >> + >> static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) >> { >> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >> @@ -353,6 +385,8 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) >> dev_dbg(mmc_dev(host->mmc), >> "%s uhs = %d, ctrl_2 = %04X\n", >> __func__, uhs, ctrl_2); >> + >> + pxav3_select_pinstate(host, uhs); > > return pxav3_select_pinstate(host, uhs);? > Its void function, so don't need it. > And could we ignore this call for those SDHCI hosts that don't need it? > I do not want to introduce flags here. And also, it is already ignored for those who don't need it. The first thing in the function pxav3_select_pinstate() is if (IS_ERR(pxa->pinctrl) || IS_ERR(pxa->pins_default) || IS_ERR(pxa->pins_fast)) return -EINVAL; So its already handled. >> } >> >> static void pxav3_voltage_switch(struct sdhci_host *host, >> @@ -416,7 +450,6 @@ static void pxav3_set_clock(struct sdhci_host *host, unsigned int clock) >> /* TX internal clock selection */ >> pxav3_set_tx_clock(host); >> } >> - > > why not fix this in patch 3? > Oops. it got missed from my eyes :) Will fix in next version. >> } >> >> static const struct sdhci_ops pxav3_sdhci_ops = { >> @@ -586,6 +619,16 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) >> } >> } >> >> + pxa->pinctrl = devm_pinctrl_get(dev); > > could we ignore this for those SDHCI hosts that don't need it? > Again, no need to introduce flags here. This is standard call and handled properly. So for the platforms not using this, it really should not matter. Also, lookup is getting executed only when pinctrl is populated. So I do not see any need here. >> + if (!IS_ERR(pxa->pinctrl)) { >> + pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default"); >> + if (IS_ERR(pxa->pins_default)) >> + dev_err(dev, "could not get default pinstate\n"); > > Why those SDHCI hosts that don't need pinctl setting should got this error? > It won't. If Host does not need pinctrl, the execution would never reach this point. The if condition check would handle it, isn't it? pxa->pinctrl = devm_pinctrl_get(dev); if (!IS_ERR(pxa->pinctrl)) { ... } So I do not see why is it issue here? Thanks, Vaibhav
On Tue, 8 Sep 2015 15:04:41 +0530 Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote: > > > On Tuesday 08 September 2015 12:22 PM, Jisheng Zhang wrote: > > On Mon, 7 Sep 2015 16:48:38 +0530 > > Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote: > > > >> Different bus clock may need different pin setting. > >> For example, fast bus clock like 208Mhz need pin drive fast > >> while slow bus clock prefer pin drive slow to guarantee > >> signal quality. > >> > >> So this patch creates two states, > >> - Default (slow/normal) pin state > >> - And fast pin state for higher freq bus speed. > >> > >> And selection of pin state is done based on timing mode. > >> > >> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org> > >> Signed-off-by: Kevin Liu <kliu5@marvell.com> > >> --- > >> drivers/mmc/host/sdhci-pxav3.c | 45 +++++++++++++++++++++++++++++++++++++++++- > >> 1 file changed, 44 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c > >> index c2b2b78..d933f75 100644 > >> --- a/drivers/mmc/host/sdhci-pxav3.c > >> +++ b/drivers/mmc/host/sdhci-pxav3.c > >> @@ -35,6 +35,7 @@ > >> #include <linux/pm.h> > >> #include <linux/pm_runtime.h> > >> #include <linux/mbus.h> > >> +#include <linux/pinctrl/consumer.h> > >> > >> #include "sdhci.h" > >> #include "sdhci-pltfm.h" > >> @@ -92,6 +93,10 @@ struct sdhci_pxa { > >> void __iomem *io_pwr_reg; > >> void __iomem *io_pwr_lock_reg; > >> struct sdhci_pxa_data *data; > >> + > >> + struct pinctrl *pinctrl; > >> + struct pinctrl_state *pins_default; > >> + struct pinctrl_state *pins_fast; > >> }; > >> > >> static struct sdhci_pxa_data pxav3_data_v1 = { > >> @@ -298,6 +303,33 @@ static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode) > >> pxa->power_mode = power_mode; > >> } > >> > >> +static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs) > >> +{ > >> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > >> + struct sdhci_pxa *pxa = pltfm_host->priv; > >> + struct pinctrl_state *pinctrl; > >> + > >> + if (IS_ERR(pxa->pinctrl) || > >> + IS_ERR(pxa->pins_default) || > >> + IS_ERR(pxa->pins_fast)) > >> + return -EINVAL; > >> + > >> + switch (uhs) { > >> + case MMC_TIMING_UHS_SDR50: > >> + case MMC_TIMING_UHS_SDR104: > >> + case MMC_TIMING_MMC_HS200: > >> + case MMC_TIMING_MMC_HS400: > >> + pinctrl = pxa->pins_fast; > >> + break; > >> + default: > >> + /* back to default state for other legacy timing */ > >> + pinctrl = pxa->pins_default; > >> + break; > >> + } > >> + > >> + return pinctrl_select_state(pxa->pinctrl, pinctrl); > >> +} > >> + > >> static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) > >> { > >> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > >> @@ -353,6 +385,8 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) > >> dev_dbg(mmc_dev(host->mmc), > >> "%s uhs = %d, ctrl_2 = %04X\n", > >> __func__, uhs, ctrl_2); > >> + > >> + pxav3_select_pinstate(host, uhs); > > > > return pxav3_select_pinstate(host, uhs);? > > > > Its void function, so don't need it. Can you please have a look at the function? static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs) > > > And could we ignore this call for those SDHCI hosts that don't need it? > > > > I do not want to introduce flags here. > And also, it is already ignored for those who don't need it. > > The first thing in the function pxav3_select_pinstate() is > > > if (IS_ERR(pxa->pinctrl) || > IS_ERR(pxa->pins_default) || > IS_ERR(pxa->pins_fast)) > return -EINVAL; > > > So its already handled. > > >> } > >> > >> static void pxav3_voltage_switch(struct sdhci_host *host, > >> @@ -416,7 +450,6 @@ static void pxav3_set_clock(struct sdhci_host *host, unsigned int clock) > >> /* TX internal clock selection */ > >> pxav3_set_tx_clock(host); > >> } > >> - > > > > why not fix this in patch 3? > > > > Oops. it got missed from my eyes :) > Will fix in next version. > > >> } > >> > >> static const struct sdhci_ops pxav3_sdhci_ops = { > >> @@ -586,6 +619,16 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) > >> } > >> } > >> > >> + pxa->pinctrl = devm_pinctrl_get(dev); > > > > could we ignore this for those SDHCI hosts that don't need it? > > > > Again, no need to introduce flags here. This is standard call and > handled properly. So for the platforms not using this, it really should > not matter. > Also, lookup is getting executed only when pinctrl is populated. > > So I do not see any need here. > > >> + if (!IS_ERR(pxa->pinctrl)) { > >> + pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default"); > >> + if (IS_ERR(pxa->pins_default)) > >> + dev_err(dev, "could not get default pinstate\n"); > > > > Why those SDHCI hosts that don't need pinctl setting should got this error? > > > > It won't. It does. On Marvell Berlin SoCs, I got [ 1.070000] sdhci-pxav3 f7ab0800.sdhci: could not get default pinstate [ 1.080000] sdhci-pxav3 f7ab0800.sdhci: could not get fast pinstate > > If Host does not need pinctrl, the execution would never reach this > point. > The if condition check would handle it, isn't it? > > pxa->pinctrl = devm_pinctrl_get(dev); It seems this function always succeed... From another side, we may have default pin in dts, for example: pin muxed between emmc and nandflash. But we don't have fast pinstate, so we at least need the flag to fast pinstate. Otherwise, in such platforms, we could get something like [ 1.000000] sdhci-pxav3 f7ab0000.sdhci: get default pinstate [ 1.000000] sdhci-pxav3 f7ab0000.sdhci: could not get fast pinstate > if (!IS_ERR(pxa->pinctrl)) { > > ... > > } > > So I do not see why is it issue here? > > > Thanks, > Vaibhav
On Tuesday 08 September 2015 12:24 PM, Jisheng Zhang wrote: > On Mon, 7 Sep 2015 16:48:38 +0530 > Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote: > >> Different bus clock may need different pin setting. >> For example, fast bus clock like 208Mhz need pin drive fast >> while slow bus clock prefer pin drive slow to guarantee >> signal quality. >> >> So this patch creates two states, >> - Default (slow/normal) pin state >> - And fast pin state for higher freq bus speed. >> >> And selection of pin state is done based on timing mode. >> >> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org> >> Signed-off-by: Kevin Liu <kliu5@marvell.com> >> --- >> drivers/mmc/host/sdhci-pxav3.c | 45 +++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 44 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c >> index c2b2b78..d933f75 100644 >> --- a/drivers/mmc/host/sdhci-pxav3.c >> +++ b/drivers/mmc/host/sdhci-pxav3.c >> @@ -35,6 +35,7 @@ >> #include <linux/pm.h> >> #include <linux/pm_runtime.h> >> #include <linux/mbus.h> >> +#include <linux/pinctrl/consumer.h> >> >> #include "sdhci.h" >> #include "sdhci-pltfm.h" >> @@ -92,6 +93,10 @@ struct sdhci_pxa { >> void __iomem *io_pwr_reg; >> void __iomem *io_pwr_lock_reg; >> struct sdhci_pxa_data *data; >> + >> + struct pinctrl *pinctrl; >> + struct pinctrl_state *pins_default; >> + struct pinctrl_state *pins_fast; >> }; >> >> static struct sdhci_pxa_data pxav3_data_v1 = { >> @@ -298,6 +303,33 @@ static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode) >> pxa->power_mode = power_mode; >> } >> >> +static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs) >> +{ >> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >> + struct sdhci_pxa *pxa = pltfm_host->priv; >> + struct pinctrl_state *pinctrl; >> + >> + if (IS_ERR(pxa->pinctrl) || >> + IS_ERR(pxa->pins_default) || >> + IS_ERR(pxa->pins_fast)) >> + return -EINVAL; >> + >> + switch (uhs) { >> + case MMC_TIMING_UHS_SDR50: >> + case MMC_TIMING_UHS_SDR104: >> + case MMC_TIMING_MMC_HS200: >> + case MMC_TIMING_MMC_HS400: > > Are you sure this IP can support HS400? > >> + pinctrl = pxa->pins_fast; No it does not. The reason I put it here is, code under discussion is related to pinctrl configuration, so for all higher bus speed mode we want to set pins into fast mode. And from hs400 perspective, I would expect it must get handled much before than this stage and should not reach here at all. So this portion of the code is complete in itself. I hope I clarified your doubt. Thanks, Vaibhav
On Tuesday 08 September 2015 03:22 PM, Jisheng Zhang wrote: > On Tue, 8 Sep 2015 15:04:41 +0530 > Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote: > >> >> >> On Tuesday 08 September 2015 12:22 PM, Jisheng Zhang wrote: >>> On Mon, 7 Sep 2015 16:48:38 +0530 >>> Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote: >>> >>>> Different bus clock may need different pin setting. >>>> For example, fast bus clock like 208Mhz need pin drive fast >>>> while slow bus clock prefer pin drive slow to guarantee >>>> signal quality. >>>> >>>> So this patch creates two states, >>>> - Default (slow/normal) pin state >>>> - And fast pin state for higher freq bus speed. >>>> >>>> And selection of pin state is done based on timing mode. >>>> >>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org> >>>> Signed-off-by: Kevin Liu <kliu5@marvell.com> >>>> --- >>>> drivers/mmc/host/sdhci-pxav3.c | 45 +++++++++++++++++++++++++++++++++++++++++- >>>> 1 file changed, 44 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c >>>> index c2b2b78..d933f75 100644 >>>> --- a/drivers/mmc/host/sdhci-pxav3.c >>>> +++ b/drivers/mmc/host/sdhci-pxav3.c >>>> @@ -35,6 +35,7 @@ >>>> #include <linux/pm.h> >>>> #include <linux/pm_runtime.h> >>>> #include <linux/mbus.h> >>>> +#include <linux/pinctrl/consumer.h> >>>> >>>> #include "sdhci.h" >>>> #include "sdhci-pltfm.h" >>>> @@ -92,6 +93,10 @@ struct sdhci_pxa { >>>> void __iomem *io_pwr_reg; >>>> void __iomem *io_pwr_lock_reg; >>>> struct sdhci_pxa_data *data; >>>> + >>>> + struct pinctrl *pinctrl; >>>> + struct pinctrl_state *pins_default; >>>> + struct pinctrl_state *pins_fast; >>>> }; >>>> >>>> static struct sdhci_pxa_data pxav3_data_v1 = { >>>> @@ -298,6 +303,33 @@ static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode) >>>> pxa->power_mode = power_mode; >>>> } >>>> >>>> +static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs) >>>> +{ >>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>> + struct sdhci_pxa *pxa = pltfm_host->priv; >>>> + struct pinctrl_state *pinctrl; >>>> + >>>> + if (IS_ERR(pxa->pinctrl) || >>>> + IS_ERR(pxa->pins_default) || >>>> + IS_ERR(pxa->pins_fast)) >>>> + return -EINVAL; >>>> + >>>> + switch (uhs) { >>>> + case MMC_TIMING_UHS_SDR50: >>>> + case MMC_TIMING_UHS_SDR104: >>>> + case MMC_TIMING_MMC_HS200: >>>> + case MMC_TIMING_MMC_HS400: >>>> + pinctrl = pxa->pins_fast; >>>> + break; >>>> + default: >>>> + /* back to default state for other legacy timing */ >>>> + pinctrl = pxa->pins_default; >>>> + break; >>>> + } >>>> + >>>> + return pinctrl_select_state(pxa->pinctrl, pinctrl); >>>> +} >>>> + >>>> static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) >>>> { >>>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>> @@ -353,6 +385,8 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) >>>> dev_dbg(mmc_dev(host->mmc), >>>> "%s uhs = %d, ctrl_2 = %04X\n", >>>> __func__, uhs, ctrl_2); >>>> + >>>> + pxav3_select_pinstate(host, uhs); >>> >>> return pxav3_select_pinstate(host, uhs);? >>> >> >> Its void function, so don't need it. > > Can you please have a look at the function? > > static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs) > I was referring to pxav3_set_uhs_signaling(). It is void, so what you are suggesting would generate warning. >> >>> And could we ignore this call for those SDHCI hosts that don't need it? >>> >> >> I do not want to introduce flags here. >> And also, it is already ignored for those who don't need it. >> >> The first thing in the function pxav3_select_pinstate() is >> >> >> if (IS_ERR(pxa->pinctrl) || >> IS_ERR(pxa->pins_default) || >> IS_ERR(pxa->pins_fast)) >> return -EINVAL; >> >> >> So its already handled. >> >>>> } >>>> >>>> static void pxav3_voltage_switch(struct sdhci_host *host, >>>> @@ -416,7 +450,6 @@ static void pxav3_set_clock(struct sdhci_host *host, unsigned int clock) >>>> /* TX internal clock selection */ >>>> pxav3_set_tx_clock(host); >>>> } >>>> - >>> >>> why not fix this in patch 3? >>> >> >> Oops. it got missed from my eyes :) >> Will fix in next version. >> >>>> } >>>> >>>> static const struct sdhci_ops pxav3_sdhci_ops = { >>>> @@ -586,6 +619,16 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) >>>> } >>>> } >>>> >>>> + pxa->pinctrl = devm_pinctrl_get(dev); >>> >>> could we ignore this for those SDHCI hosts that don't need it? >>> >> >> Again, no need to introduce flags here. This is standard call and >> handled properly. So for the platforms not using this, it really should >> not matter. >> Also, lookup is getting executed only when pinctrl is populated. >> >> So I do not see any need here. >> >>>> + if (!IS_ERR(pxa->pinctrl)) { >>>> + pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default"); >>>> + if (IS_ERR(pxa->pins_default)) >>>> + dev_err(dev, "could not get default pinstate\n"); >>> >>> Why those SDHCI hosts that don't need pinctl setting should got this error? >>> >> >> It won't. > > It does. On Marvell Berlin SoCs, I got > > [ 1.070000] sdhci-pxav3 f7ab0800.sdhci: could not get default pinstate > [ 1.080000] sdhci-pxav3 f7ab0800.sdhci: could not get fast pinstate > > > >> >> If Host does not need pinctrl, the execution would never reach this >> point. >> The if condition check would handle it, isn't it? >> >> pxa->pinctrl = devm_pinctrl_get(dev); > > It seems this function always succeed... > Not always. I would succeed only if you have pinctrl defined in DT for this device. And if you have pinctrl defined, isn't it is expected to have "default" pin state to be always present? And if answer is yes here, then it is fair to be prompting error for it. > From another side, we may have default pin in dts, for example: pin muxed between > emmc and nandflash. But we don't have fast pinstate, so we at least need the > flag to fast pinstate. Otherwise, in such platforms, we could get something like > That is exactly the reason behind keeping it as dev_info. > [ 1.000000] sdhci-pxav3 f7ab0000.sdhci: get default pinstate > [ 1.000000] sdhci-pxav3 f7ab0000.sdhci: could not get fast pinstate > > Thanks, Vaibhav
On Tue, 8 Sep 2015 15:32:34 +0530 Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote: > > > On Tuesday 08 September 2015 03:22 PM, Jisheng Zhang wrote: > > On Tue, 8 Sep 2015 15:04:41 +0530 > > Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote: > > > >> > >> > >> On Tuesday 08 September 2015 12:22 PM, Jisheng Zhang wrote: > >>> On Mon, 7 Sep 2015 16:48:38 +0530 > >>> Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote: > >>> > >>>> Different bus clock may need different pin setting. > >>>> For example, fast bus clock like 208Mhz need pin drive fast > >>>> while slow bus clock prefer pin drive slow to guarantee > >>>> signal quality. > >>>> > >>>> So this patch creates two states, > >>>> - Default (slow/normal) pin state > >>>> - And fast pin state for higher freq bus speed. > >>>> > >>>> And selection of pin state is done based on timing mode. > >>>> > >>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org> > >>>> Signed-off-by: Kevin Liu <kliu5@marvell.com> > >>>> --- > >>>> drivers/mmc/host/sdhci-pxav3.c | 45 +++++++++++++++++++++++++++++++++++++++++- > >>>> 1 file changed, 44 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c > >>>> index c2b2b78..d933f75 100644 > >>>> --- a/drivers/mmc/host/sdhci-pxav3.c > >>>> +++ b/drivers/mmc/host/sdhci-pxav3.c > >>>> @@ -35,6 +35,7 @@ > >>>> #include <linux/pm.h> > >>>> #include <linux/pm_runtime.h> > >>>> #include <linux/mbus.h> > >>>> +#include <linux/pinctrl/consumer.h> > >>>> > >>>> #include "sdhci.h" > >>>> #include "sdhci-pltfm.h" > >>>> @@ -92,6 +93,10 @@ struct sdhci_pxa { > >>>> void __iomem *io_pwr_reg; > >>>> void __iomem *io_pwr_lock_reg; > >>>> struct sdhci_pxa_data *data; > >>>> + > >>>> + struct pinctrl *pinctrl; > >>>> + struct pinctrl_state *pins_default; > >>>> + struct pinctrl_state *pins_fast; > >>>> }; > >>>> > >>>> static struct sdhci_pxa_data pxav3_data_v1 = { > >>>> @@ -298,6 +303,33 @@ static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode) > >>>> pxa->power_mode = power_mode; > >>>> } > >>>> > >>>> +static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs) > >>>> +{ > >>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > >>>> + struct sdhci_pxa *pxa = pltfm_host->priv; > >>>> + struct pinctrl_state *pinctrl; > >>>> + > >>>> + if (IS_ERR(pxa->pinctrl) || > >>>> + IS_ERR(pxa->pins_default) || > >>>> + IS_ERR(pxa->pins_fast)) > >>>> + return -EINVAL; > >>>> + > >>>> + switch (uhs) { > >>>> + case MMC_TIMING_UHS_SDR50: > >>>> + case MMC_TIMING_UHS_SDR104: > >>>> + case MMC_TIMING_MMC_HS200: > >>>> + case MMC_TIMING_MMC_HS400: > >>>> + pinctrl = pxa->pins_fast; > >>>> + break; > >>>> + default: > >>>> + /* back to default state for other legacy timing */ > >>>> + pinctrl = pxa->pins_default; > >>>> + break; > >>>> + } > >>>> + > >>>> + return pinctrl_select_state(pxa->pinctrl, pinctrl); > >>>> +} > >>>> + > >>>> static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) > >>>> { > >>>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > >>>> @@ -353,6 +385,8 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) > >>>> dev_dbg(mmc_dev(host->mmc), > >>>> "%s uhs = %d, ctrl_2 = %04X\n", > >>>> __func__, uhs, ctrl_2); > >>>> + > >>>> + pxav3_select_pinstate(host, uhs); > >>> > >>> return pxav3_select_pinstate(host, uhs);? > >>> > >> > >> Its void function, so don't need it. > > > > Can you please have a look at the function? > > > > static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs) > > > > I was referring to pxav3_set_uhs_signaling(). > It is void, so what you are suggesting would generate warning. Oops, you are correct. > > >> > >>> And could we ignore this call for those SDHCI hosts that don't need it? > >>> > >> > >> I do not want to introduce flags here. > >> And also, it is already ignored for those who don't need it. > >> > >> The first thing in the function pxav3_select_pinstate() is > >> > >> > >> if (IS_ERR(pxa->pinctrl) || > >> IS_ERR(pxa->pins_default) || > >> IS_ERR(pxa->pins_fast)) > >> return -EINVAL; > >> > >> > >> So its already handled. > >> > >>>> } > >>>> > >>>> static void pxav3_voltage_switch(struct sdhci_host *host, > >>>> @@ -416,7 +450,6 @@ static void pxav3_set_clock(struct sdhci_host *host, unsigned int clock) > >>>> /* TX internal clock selection */ > >>>> pxav3_set_tx_clock(host); > >>>> } > >>>> - > >>> > >>> why not fix this in patch 3? > >>> > >> > >> Oops. it got missed from my eyes :) > >> Will fix in next version. > >> > >>>> } > >>>> > >>>> static const struct sdhci_ops pxav3_sdhci_ops = { > >>>> @@ -586,6 +619,16 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) > >>>> } > >>>> } > >>>> > >>>> + pxa->pinctrl = devm_pinctrl_get(dev); > >>> > >>> could we ignore this for those SDHCI hosts that don't need it? > >>> > >> > >> Again, no need to introduce flags here. This is standard call and > >> handled properly. So for the platforms not using this, it really should > >> not matter. > >> Also, lookup is getting executed only when pinctrl is populated. > >> > >> So I do not see any need here. > >> > >>>> + if (!IS_ERR(pxa->pinctrl)) { > >>>> + pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default"); > >>>> + if (IS_ERR(pxa->pins_default)) > >>>> + dev_err(dev, "could not get default pinstate\n"); > >>> > >>> Why those SDHCI hosts that don't need pinctl setting should got this error? > >>> > >> > >> It won't. > > > > It does. On Marvell Berlin SoCs, I got > > > > [ 1.070000] sdhci-pxav3 f7ab0800.sdhci: could not get default pinstate > > [ 1.080000] sdhci-pxav3 f7ab0800.sdhci: could not get fast pinstate > > > > > > > >> > >> If Host does not need pinctrl, the execution would never reach this > >> point. > >> The if condition check would handle it, isn't it? > >> > >> pxa->pinctrl = devm_pinctrl_get(dev); > > > > It seems this function always succeed... > > > > Not always. > I would succeed only if you have pinctrl defined in DT for this device. Yes, that's what I thought, but I got [ 1.070000] sdhci-pxav3 f7ab0800.sdhci: could not get default pinstate [ 1.080000] sdhci-pxav3 f7ab0800.sdhci: could not get fast pinstate there's no pinctrl for f7ab0800.sdhci. Am I missing somthing? Thanks, Jisheng > > And if you have pinctrl defined, isn't it is expected to have "default" > pin state to be always present? > And if answer is yes here, then it is fair to be prompting error for it. > > > From another side, we may have default pin in dts, for example: pin muxed between > > emmc and nandflash. But we don't have fast pinstate, so we at least need the > > flag to fast pinstate. Otherwise, in such platforms, we could get something like > > > > That is exactly the reason behind keeping it as dev_info. > > > [ 1.000000] sdhci-pxav3 f7ab0000.sdhci: get default pinstate > > [ 1.000000] sdhci-pxav3 f7ab0000.sdhci: could not get fast pinstate > > > > > Thanks, > Vaibhav
On Tuesday 08 September 2015 03:34 PM, Jisheng Zhang wrote: > On Tue, 8 Sep 2015 15:32:34 +0530 > Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote: > >> >> >> On Tuesday 08 September 2015 03:22 PM, Jisheng Zhang wrote: >>> On Tue, 8 Sep 2015 15:04:41 +0530 >>> Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote: >>> <snip> >>>>>> static const struct sdhci_ops pxav3_sdhci_ops = { >>>>>> @@ -586,6 +619,16 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) >>>>>> } >>>>>> } >>>>>> >>>>>> + pxa->pinctrl = devm_pinctrl_get(dev); >>>>> >>>>> could we ignore this for those SDHCI hosts that don't need it? >>>>> >>>> >>>> Again, no need to introduce flags here. This is standard call and >>>> handled properly. So for the platforms not using this, it really should >>>> not matter. >>>> Also, lookup is getting executed only when pinctrl is populated. >>>> >>>> So I do not see any need here. >>>> >>>>>> + if (!IS_ERR(pxa->pinctrl)) { >>>>>> + pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default"); >>>>>> + if (IS_ERR(pxa->pins_default)) >>>>>> + dev_err(dev, "could not get default pinstate\n"); >>>>> >>>>> Why those SDHCI hosts that don't need pinctl setting should got this error? >>>>> >>>> >>>> It won't. >>> >>> It does. On Marvell Berlin SoCs, I got >>> >>> [ 1.070000] sdhci-pxav3 f7ab0800.sdhci: could not get default pinstate >>> [ 1.080000] sdhci-pxav3 f7ab0800.sdhci: could not get fast pinstate >>> >>> >>> >>>> >>>> If Host does not need pinctrl, the execution would never reach this >>>> point. >>>> The if condition check would handle it, isn't it? >>>> >>>> pxa->pinctrl = devm_pinctrl_get(dev); >>> >>> It seems this function always succeed... >>> >> >> Not always. >> I would succeed only if you have pinctrl defined in DT for this device. > > Yes, that's what I thought, but I got > > [ 1.070000] sdhci-pxav3 f7ab0800.sdhci: could not get default pinstate > [ 1.080000] sdhci-pxav3 f7ab0800.sdhci: could not get fast pinstate > > there's no pinctrl for f7ab0800.sdhci. Am I missing somthing? > > Thanks, > Jisheng > >> >> And if you have pinctrl defined, isn't it is expected to have "default" >> pin state to be always present? >> And if answer is yes here, then it is fair to be prompting error for it. >> >>> From another side, we may have default pin in dts, for example: pin muxed between >>> emmc and nandflash. But we don't have fast pinstate, so we at least need the >>> flag to fast pinstate. Otherwise, in such platforms, we could get something like >>> >> >> That is exactly the reason behind keeping it as dev_info. >> >>> [ 1.000000] sdhci-pxav3 f7ab0000.sdhci: get default pinstate >>> [ 1.000000] sdhci-pxav3 f7ab0000.sdhci: could not get fast pinstate >>> >>> I did some invastigation here on the execution flow, and you know what, you are right here. It seems, devm_pinctrl_get() always returns valid pinctrl pointer, even though the DT property is not populated. The return value from I did some invastigation () should have been treated differently, but it is not. Instead it creates the "struct pinctrl" and return back to the driver. I am looping Linus Walleji here, probably he can comment/confirm on this. Thanks, Vaibhav >> Thanks, >> Vaibhav >
On Mon, Sep 7, 2015 at 1:18 PM, Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote: > Different bus clock may need different pin setting. > For example, fast bus clock like 208Mhz need pin drive fast > while slow bus clock prefer pin drive slow to guarantee > signal quality. > > So this patch creates two states, > - Default (slow/normal) pin state > - And fast pin state for higher freq bus speed. > > And selection of pin state is done based on timing mode. > > Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org> > Signed-off-by: Kevin Liu <kliu5@marvell.com> (...) > + pxa->pinctrl = devm_pinctrl_get(dev); > + if (!IS_ERR(pxa->pinctrl)) { > + pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default"); > + if (IS_ERR(pxa->pins_default)) > + dev_err(dev, "could not get default pinstate\n"); > + pxa->pins_fast = pinctrl_lookup_state(pxa->pinctrl, "fast"); > + if (IS_ERR(pxa->pins_fast)) > + dev_info(dev, "could not get fast pinstate\n"); > + } This is exactly how I think it should be used from a pin control point of view. If you depended on CONFIG_PM you could use pinctrl_pm_select_default_state() but for this simple scenario this is fine. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> From a pinctrl point of view. Yours, Linus Walleij
On Tuesday 08 September 2015 08:12 PM, Linus Walleij wrote: > On Mon, Sep 7, 2015 at 1:18 PM, Vaibhav Hiremath > <vaibhav.hiremath@linaro.org> wrote: > >> Different bus clock may need different pin setting. >> For example, fast bus clock like 208Mhz need pin drive fast >> while slow bus clock prefer pin drive slow to guarantee >> signal quality. >> >> So this patch creates two states, >> - Default (slow/normal) pin state >> - And fast pin state for higher freq bus speed. >> >> And selection of pin state is done based on timing mode. >> >> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org> >> Signed-off-by: Kevin Liu <kliu5@marvell.com> > (...) >> + pxa->pinctrl = devm_pinctrl_get(dev); >> + if (!IS_ERR(pxa->pinctrl)) { >> + pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default"); >> + if (IS_ERR(pxa->pins_default)) >> + dev_err(dev, "could not get default pinstate\n"); >> + pxa->pins_fast = pinctrl_lookup_state(pxa->pinctrl, "fast"); >> + if (IS_ERR(pxa->pins_fast)) >> + dev_info(dev, "could not get fast pinstate\n"); >> + } > > This is exactly how I think it should be used from a pin control > point of view. > > If you depended on CONFIG_PM you could use > pinctrl_pm_select_default_state() but for this simple scenario > this is fine. > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > From a pinctrl point of view. > Thanks for your review. Linus, I agree this is how it should be used. But I still have one small doubt on expectation from devm_pinctrl_get() function. If pinctrl properties are not populated in Devicetree node, then, shouldn't devm_pinctrl_get() return error ? I followed the code flow, and it seems even if pinctrl properties are not populated in DT node, the devm_pinctrl_get() returns valid pointer to "struct pinctrl", isn't this against the expectation of the call? Code flow - devm_pinctrl_get() ... --> creat_pinctrl() --> pinctrl_dt_to_map() ... pinctrl_dt_to_map() iterates for pinctrl-x (x = 0,1,...) and if it founds the entry then it parses the node. If it doesn't find any pinctrl property then also it returns 0. and subsequently rreturns handle to "struct pinctrl" for the device. Why is so? Thanks, Vaibhav
On Tue, Sep 8, 2015 at 5:07 PM, Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote: > But I still have one small doubt on expectation from > devm_pinctrl_get() function. > > If pinctrl properties are not populated in Devicetree node, > then, shouldn't devm_pinctrl_get() return error ? > I followed the code flow, and it seems even if pinctrl properties are > not populated in DT node, the devm_pinctrl_get() returns valid > pointer to "struct pinctrl", isn't this against the expectation of the > call? > > > Code flow - > > devm_pinctrl_get() > ... > --> creat_pinctrl() That is the spelling mistake Dennis Ritchie and Ken Thompson wish they had avoided in the first syscall interface :D > --> pinctrl_dt_to_map() > ... > > pinctrl_dt_to_map() iterates for pinctrl-x (x = 0,1,...) and if it > founds the entry then it parses the node. If it doesn't find any > pinctrl property then also it returns 0. and subsequently rreturns > handle to "struct pinctrl" for the device. Why is so? So pinctrl_dt_to_map() returns 0 if there are no maps in the device tree. This is correct: there may be systems that have a mixture of device tree and platform data, and in that case the code needs to continue after calling pinctrl_dt_to_map() because else it bails out before coming to the loop in create_pinctrl() where we iterate over the static maps. So after the loop in create_pinctrl() it should be possible to return something like -ENOENT if we didn't find anything. The device core pinctrl handling in drivers/base/pinctrl.c seems to cope with it. It's a semantic change so we would need to test to toss it in and see what happens on some different systems but in principe I think you're right. What we get if there is no state is basically a dummy pinctrl that does nothing. Do you wanna make a patch for this? (Looping in Stephen Warren so he can tell if I miss something obviously evident in the design that require stubs to be present.) Yours, Linus Walleij
diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c index c2b2b78..d933f75 100644 --- a/drivers/mmc/host/sdhci-pxav3.c +++ b/drivers/mmc/host/sdhci-pxav3.c @@ -35,6 +35,7 @@ #include <linux/pm.h> #include <linux/pm_runtime.h> #include <linux/mbus.h> +#include <linux/pinctrl/consumer.h> #include "sdhci.h" #include "sdhci-pltfm.h" @@ -92,6 +93,10 @@ struct sdhci_pxa { void __iomem *io_pwr_reg; void __iomem *io_pwr_lock_reg; struct sdhci_pxa_data *data; + + struct pinctrl *pinctrl; + struct pinctrl_state *pins_default; + struct pinctrl_state *pins_fast; }; static struct sdhci_pxa_data pxav3_data_v1 = { @@ -298,6 +303,33 @@ static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode) pxa->power_mode = power_mode; } +static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs) +{ + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct sdhci_pxa *pxa = pltfm_host->priv; + struct pinctrl_state *pinctrl; + + if (IS_ERR(pxa->pinctrl) || + IS_ERR(pxa->pins_default) || + IS_ERR(pxa->pins_fast)) + return -EINVAL; + + switch (uhs) { + case MMC_TIMING_UHS_SDR50: + case MMC_TIMING_UHS_SDR104: + case MMC_TIMING_MMC_HS200: + case MMC_TIMING_MMC_HS400: + pinctrl = pxa->pins_fast; + break; + default: + /* back to default state for other legacy timing */ + pinctrl = pxa->pins_default; + break; + } + + return pinctrl_select_state(pxa->pinctrl, pinctrl); +} + static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) { struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); @@ -353,6 +385,8 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) dev_dbg(mmc_dev(host->mmc), "%s uhs = %d, ctrl_2 = %04X\n", __func__, uhs, ctrl_2); + + pxav3_select_pinstate(host, uhs); } static void pxav3_voltage_switch(struct sdhci_host *host, @@ -416,7 +450,6 @@ static void pxav3_set_clock(struct sdhci_host *host, unsigned int clock) /* TX internal clock selection */ pxav3_set_tx_clock(host); } - } static const struct sdhci_ops pxav3_sdhci_ops = { @@ -586,6 +619,16 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) } } + pxa->pinctrl = devm_pinctrl_get(dev); + if (!IS_ERR(pxa->pinctrl)) { + pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default"); + if (IS_ERR(pxa->pins_default)) + dev_err(dev, "could not get default pinstate\n"); + pxa->pins_fast = pinctrl_lookup_state(pxa->pinctrl, "fast"); + if (IS_ERR(pxa->pins_fast)) + dev_info(dev, "could not get fast pinstate\n"); + } + pm_runtime_get_noresume(&pdev->dev); pm_runtime_set_active(&pdev->dev); pm_runtime_set_autosuspend_delay(&pdev->dev, PXAV3_RPM_DELAY_MS);