Message ID | 181127e4ec8bbfbf0f80018ff157b59d63597c44.1460986983.git.larper@axis.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 18 April 2016 at 15:44, Lars Persson <lars.persson@axis.com> wrote: > Some boards need different pin drive strength for the UHS mode. Add an > optional pinctrl setting with two pin states covering UHS speeds and > other speeds. > > Signed-off-by: Lars Persson <larper@axis.com> > --- > drivers/mmc/host/usdhi6rol0.c | 40 +++++++++++++++++++++++++++++++++++++++- > 1 file changed, 39 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/usdhi6rol0.c b/drivers/mmc/host/usdhi6rol0.c > index 2585ea4..e8a5c8f 100644 > --- a/drivers/mmc/host/usdhi6rol0.c > +++ b/drivers/mmc/host/usdhi6rol0.c > @@ -22,6 +22,7 @@ > #include <linux/mmc/sdio.h> > #include <linux/module.h> > #include <linux/pagemap.h> > +#include <linux/pinctrl/consumer.h> > #include <linux/platform_device.h> > #include <linux/scatterlist.h> > #include <linux/string.h> > @@ -198,6 +199,11 @@ struct usdhi6_host { > struct dma_chan *chan_rx; > struct dma_chan *chan_tx; > bool dma_active; > + > + /* Pin control */ > + struct pinctrl *pinctrl; > + struct pinctrl_state *pins_default; > + struct pinctrl_state *pins_uhs; > }; > > /* I/O primitives */ > @@ -1147,14 +1153,39 @@ static void usdhi6_enable_sdio_irq(struct mmc_host *mmc, int enable) > } > } > > +static int usdhi6_set_pinstates(struct usdhi6_host *host, int voltage) > +{ > + if (IS_ERR(host->pinctrl) || IS_ERR(host->pins_default) || > + IS_ERR(host->pins_uhs)) So this means, if you have UHS support you need both default and uhs pins. That makes sense but it's not according to the DT documentation in patch 1/3. > + return 0; > + > + switch (voltage) { > + case MMC_SIGNAL_VOLTAGE_180: > + case MMC_SIGNAL_VOLTAGE_120: > + return pinctrl_select_state(host->pinctrl, > + host->pins_uhs); > + > + default: > + return pinctrl_select_state(host->pinctrl, > + host->pins_default); > + } > +} > + > static int usdhi6_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios) > { > int ret; > > ret = mmc_regulator_set_vqmmc(mmc, ios); What if you don't have pins for default and uhs state? Should you verify that's the case before deciding to switch voltage? > > - if (ret < 0) > + if (ret < 0) { > dev_warn(mmc_dev(mmc), "Voltage switch failed: %d\n", ret); > + return ret; > + } > + > + ret = usdhi6_set_pinstates(mmc_priv(mmc), ios->signal_voltage); > + if (ret) > + dev_warn_once(mmc_dev(mmc), > + "Failed to set pinstate err=%d\n", ret); > > return ret; > } > @@ -1817,6 +1848,13 @@ static int usdhi6_probe(struct platform_device *pdev) > mmc->f_max = host->imclk; > mmc->f_min = host->imclk / 512; > > + host->pinctrl = devm_pinctrl_get(&pdev->dev); > + if (!IS_ERR(host->pinctrl)) { > + host->pins_default = pinctrl_lookup_state(host->pinctrl, > + PINCTRL_STATE_DEFAULT); According to the DT documentation in patch 1/3, the default pins should be required and not optional. I assume you want them to be optional, but required when supporting UHS, right? > + host->pins_uhs = pinctrl_lookup_state(host->pinctrl, > + "pins_uhs"); > + } > platform_set_drvdata(pdev, host); > > ret = mmc_add_host(mmc); > -- > 2.1.4 > Kind regards Uffe -- 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
> 19 apr. 2016 at 11:45 Ulf Hansson <ulf.hansson@linaro.org> wrote: > >> On 18 April 2016 at 15:44, Lars Persson <lars.persson@axis.com> wrote: >> Some boards need different pin drive strength for the UHS mode. Add an >> optional pinctrl setting with two pin states covering UHS speeds and >> other speeds. >> >> Signed-off-by: Lars Persson <larper@axis.com> >> --- >> drivers/mmc/host/usdhi6rol0.c | 40 +++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 39 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/host/usdhi6rol0.c b/drivers/mmc/host/usdhi6rol0.c >> index 2585ea4..e8a5c8f 100644 >> --- a/drivers/mmc/host/usdhi6rol0.c >> +++ b/drivers/mmc/host/usdhi6rol0.c >> @@ -22,6 +22,7 @@ >> #include <linux/mmc/sdio.h> >> #include <linux/module.h> >> #include <linux/pagemap.h> >> +#include <linux/pinctrl/consumer.h> >> #include <linux/platform_device.h> >> #include <linux/scatterlist.h> >> #include <linux/string.h> >> @@ -198,6 +199,11 @@ struct usdhi6_host { >> struct dma_chan *chan_rx; >> struct dma_chan *chan_tx; >> bool dma_active; >> + >> + /* Pin control */ >> + struct pinctrl *pinctrl; >> + struct pinctrl_state *pins_default; >> + struct pinctrl_state *pins_uhs; >> }; >> >> /* I/O primitives */ >> @@ -1147,14 +1153,39 @@ static void usdhi6_enable_sdio_irq(struct mmc_host *mmc, int enable) >> } >> } >> >> +static int usdhi6_set_pinstates(struct usdhi6_host *host, int voltage) >> +{ >> + if (IS_ERR(host->pinctrl) || IS_ERR(host->pins_default) || >> + IS_ERR(host->pins_uhs)) > > So this means, if you have UHS support you need both default and uhs > pins. That makes sense but it's not according to the DT documentation > in patch 1/3. No. It means that if this particular board requires distinct settings for UHS then both pin states need to be defined. Otherwise we assume that the default settings are fine also for UHS. > >> + return 0; >> + >> + switch (voltage) { >> + case MMC_SIGNAL_VOLTAGE_180: >> + case MMC_SIGNAL_VOLTAGE_120: >> + return pinctrl_select_state(host->pinctrl, >> + host->pins_uhs); >> + >> + default: >> + return pinctrl_select_state(host->pinctrl, >> + host->pins_default); >> + } >> +} >> + >> static int usdhi6_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios) >> { >> int ret; >> >> ret = mmc_regulator_set_vqmmc(mmc, ios); > > What if you don't have pins for default and uhs state? Should you > verify that's the case before deciding to switch voltage No see my reply above. > >> >> - if (ret < 0) >> + if (ret < 0) { >> dev_warn(mmc_dev(mmc), "Voltage switch failed: %d\n", ret); >> + return ret; >> + } >> + >> + ret = usdhi6_set_pinstates(mmc_priv(mmc), ios->signal_voltage); >> + if (ret) >> + dev_warn_once(mmc_dev(mmc), >> + "Failed to set pinstate err=%d\n", ret); >> >> return ret; >> } >> @@ -1817,6 +1848,13 @@ static int usdhi6_probe(struct platform_device *pdev) >> mmc->f_max = host->imclk; >> mmc->f_min = host->imclk / 512; >> >> + host->pinctrl = devm_pinctrl_get(&pdev->dev); >> + if (!IS_ERR(host->pinctrl)) { >> + host->pins_default = pinctrl_lookup_state(host->pinctrl, >> + PINCTRL_STATE_DEFAULT); > > According to the DT documentation in patch 1/3, the default pins > should be required and not optional. I assume you want them to be > optional, but required when supporting UHS, right? Yes the default is required only when a UHS pin state is defined. Hence all the pinctrl parameters are listed as optional, but the presence of a pinctrl-names parameter makes the default pins required. Thanks. - Lars-- 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
[...] >>> >>> +static int usdhi6_set_pinstates(struct usdhi6_host *host, int voltage) >>> +{ >>> + if (IS_ERR(host->pinctrl) || IS_ERR(host->pins_default) || >>> + IS_ERR(host->pins_uhs)) >> >> So this means, if you have UHS support you need both default and uhs >> pins. That makes sense but it's not according to the DT documentation >> in patch 1/3. > > No. It means that if this particular board requires distinct settings for UHS then both pin states need to be defined. Otherwise we assume that the default settings are fine also for UHS. I see. I would rather see that you bail out in ->probe() when IS_ERR(host->pinctrl) instead of testing for that here. Conforming to the policy you describe here, you should only need to test for IS_ERR(host->pins_uhs), as host->pins_default shall then be implicitly covered by that test. > >> >>> + return 0; >>> + >>> + switch (voltage) { >>> + case MMC_SIGNAL_VOLTAGE_180: >>> + case MMC_SIGNAL_VOLTAGE_120: >>> + return pinctrl_select_state(host->pinctrl, >>> + host->pins_uhs); >>> + >>> + default: >>> + return pinctrl_select_state(host->pinctrl, >>> + host->pins_default); >>> + } >>> +} >>> + >>> static int usdhi6_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios) >>> { >>> int ret; >>> >>> ret = mmc_regulator_set_vqmmc(mmc, ios); >> >> What if you don't have pins for default and uhs state? Should you >> verify that's the case before deciding to switch voltage > > No see my reply above. Okay! > >> >>> >>> - if (ret < 0) >>> + if (ret < 0) { >>> dev_warn(mmc_dev(mmc), "Voltage switch failed: %d\n", ret); >>> + return ret; >>> + } >>> + >>> + ret = usdhi6_set_pinstates(mmc_priv(mmc), ios->signal_voltage); >>> + if (ret) >>> + dev_warn_once(mmc_dev(mmc), >>> + "Failed to set pinstate err=%d\n", ret); >>> >>> return ret; >>> } >>> @@ -1817,6 +1848,13 @@ static int usdhi6_probe(struct platform_device *pdev) >>> mmc->f_max = host->imclk; >>> mmc->f_min = host->imclk / 512; >>> >>> + host->pinctrl = devm_pinctrl_get(&pdev->dev); >>> + if (!IS_ERR(host->pinctrl)) { >>> + host->pins_default = pinctrl_lookup_state(host->pinctrl, >>> + PINCTRL_STATE_DEFAULT); >> >> According to the DT documentation in patch 1/3, the default pins >> should be required and not optional. I assume you want them to be >> optional, but required when supporting UHS, right? > > Yes the default is required only when a UHS pin state is defined. Hence all the pinctrl parameters are listed as optional, but the presence of a pinctrl-names parameter makes the default pins required. I am not sure why you say default state is required when pinctrl-names is defined. It's rather a recommendation and it's being selected by the driver core if it exist. Please update the patch for DT documentation to reflect the policy. Moreover you need to deploy this policy properly here and you also need to add error handling. In more detail. 1. Check for errors from devm_pinctrl_get() and propagate them. 2. Check if "state_uhs" can be fetched and if so try also to fetch default state. If the latter fails you need propagate that as an error as well. Does that make sense? Kind regards Uffe -- 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 04/20/2016 02:15 PM, Ulf Hansson wrote: > [...] > >>>> >>>> +static int usdhi6_set_pinstates(struct usdhi6_host *host, int voltage) >>>> +{ >>>> + if (IS_ERR(host->pinctrl) || IS_ERR(host->pins_default) || >>>> + IS_ERR(host->pins_uhs)) >>> >>> So this means, if you have UHS support you need both default and uhs >>> pins. That makes sense but it's not according to the DT documentation >>> in patch 1/3. >> >> No. It means that if this particular board requires distinct settings for UHS then both pin states need to be defined. Otherwise we assume that the default settings are fine also for UHS. > > I see. > > I would rather see that you bail out in ->probe() when > IS_ERR(host->pinctrl) instead of testing for that here. > > Conforming to the policy you describe here, you should only need to > test for IS_ERR(host->pins_uhs), as host->pins_default shall then be > implicitly covered by that test. Ok. > >> >>> >>>> + return 0; >>>> + >>>> + switch (voltage) { >>>> + case MMC_SIGNAL_VOLTAGE_180: >>>> + case MMC_SIGNAL_VOLTAGE_120: >>>> + return pinctrl_select_state(host->pinctrl, >>>> + host->pins_uhs); >>>> + >>>> + default: >>>> + return pinctrl_select_state(host->pinctrl, >>>> + host->pins_default); >>>> + } >>>> +} >>>> + >>>> static int usdhi6_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios) >>>> { >>>> int ret; >>>> >>>> ret = mmc_regulator_set_vqmmc(mmc, ios); >>> >>> What if you don't have pins for default and uhs state? Should you >>> verify that's the case before deciding to switch voltage >> >> No see my reply above. > > Okay! > >> >>> >>>> >>>> - if (ret < 0) >>>> + if (ret < 0) { >>>> dev_warn(mmc_dev(mmc), "Voltage switch failed: %d\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + ret = usdhi6_set_pinstates(mmc_priv(mmc), ios->signal_voltage); >>>> + if (ret) >>>> + dev_warn_once(mmc_dev(mmc), >>>> + "Failed to set pinstate err=%d\n", ret); >>>> >>>> return ret; >>>> } >>>> @@ -1817,6 +1848,13 @@ static int usdhi6_probe(struct platform_device *pdev) >>>> mmc->f_max = host->imclk; >>>> mmc->f_min = host->imclk / 512; >>>> >>>> + host->pinctrl = devm_pinctrl_get(&pdev->dev); >>>> + if (!IS_ERR(host->pinctrl)) { >>>> + host->pins_default = pinctrl_lookup_state(host->pinctrl, >>>> + PINCTRL_STATE_DEFAULT); >>> >>> According to the DT documentation in patch 1/3, the default pins >>> should be required and not optional. I assume you want them to be >>> optional, but required when supporting UHS, right? >> >> Yes the default is required only when a UHS pin state is defined. Hence all the pinctrl parameters are listed as optional, but the presence of a pinctrl-names parameter makes the default pins required. > > I am not sure why you say default state is required when pinctrl-names > is defined. It's rather a recommendation and it's being selected by > the driver core if it exist. I considered the two use-cases for pinctrl in this driver: - Only using default and letting the core select it. - Using it for UHS in which case default and pins_uhs are required. Both cases need the default, that is why it was listed as required. But I will change this based on your feedback. > > Please update the patch for DT documentation to reflect the policy. > Moreover you need to deploy this policy properly here and you also > need to add error handling. > > In more detail. > 1. Check for errors from devm_pinctrl_get() and propagate them. > 2. Check if "state_uhs" can be fetched and if so try also to fetch > default state. If the latter fails you need propagate that as an error > as well. > > Does that make sense? OK, I will implement this error checking. - Lars -- 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
diff --git a/drivers/mmc/host/usdhi6rol0.c b/drivers/mmc/host/usdhi6rol0.c index 2585ea4..e8a5c8f 100644 --- a/drivers/mmc/host/usdhi6rol0.c +++ b/drivers/mmc/host/usdhi6rol0.c @@ -22,6 +22,7 @@ #include <linux/mmc/sdio.h> #include <linux/module.h> #include <linux/pagemap.h> +#include <linux/pinctrl/consumer.h> #include <linux/platform_device.h> #include <linux/scatterlist.h> #include <linux/string.h> @@ -198,6 +199,11 @@ struct usdhi6_host { struct dma_chan *chan_rx; struct dma_chan *chan_tx; bool dma_active; + + /* Pin control */ + struct pinctrl *pinctrl; + struct pinctrl_state *pins_default; + struct pinctrl_state *pins_uhs; }; /* I/O primitives */ @@ -1147,14 +1153,39 @@ static void usdhi6_enable_sdio_irq(struct mmc_host *mmc, int enable) } } +static int usdhi6_set_pinstates(struct usdhi6_host *host, int voltage) +{ + if (IS_ERR(host->pinctrl) || IS_ERR(host->pins_default) || + IS_ERR(host->pins_uhs)) + return 0; + + switch (voltage) { + case MMC_SIGNAL_VOLTAGE_180: + case MMC_SIGNAL_VOLTAGE_120: + return pinctrl_select_state(host->pinctrl, + host->pins_uhs); + + default: + return pinctrl_select_state(host->pinctrl, + host->pins_default); + } +} + static int usdhi6_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios) { int ret; ret = mmc_regulator_set_vqmmc(mmc, ios); - if (ret < 0) + if (ret < 0) { dev_warn(mmc_dev(mmc), "Voltage switch failed: %d\n", ret); + return ret; + } + + ret = usdhi6_set_pinstates(mmc_priv(mmc), ios->signal_voltage); + if (ret) + dev_warn_once(mmc_dev(mmc), + "Failed to set pinstate err=%d\n", ret); return ret; } @@ -1817,6 +1848,13 @@ static int usdhi6_probe(struct platform_device *pdev) mmc->f_max = host->imclk; mmc->f_min = host->imclk / 512; + host->pinctrl = devm_pinctrl_get(&pdev->dev); + if (!IS_ERR(host->pinctrl)) { + host->pins_default = pinctrl_lookup_state(host->pinctrl, + PINCTRL_STATE_DEFAULT); + host->pins_uhs = pinctrl_lookup_state(host->pinctrl, + "pins_uhs"); + } platform_set_drvdata(pdev, host); ret = mmc_add_host(mmc);
Some boards need different pin drive strength for the UHS mode. Add an optional pinctrl setting with two pin states covering UHS speeds and other speeds. Signed-off-by: Lars Persson <larper@axis.com> --- drivers/mmc/host/usdhi6rol0.c | 40 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-)