On Tue, 8 Sep 2015 15:04:41 +0530
Vaibhav Hiremath <vaibhav.hirem...@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.hirem...@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.hirem...@linaro.org>
> >> Signed-off-by: Kevin Liu <kl...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
