| Message ID | 1370546059-24181-7-git-send-email-balajitk@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Balaji T K <balajitk@ti.com> [130606 12:20]: > PBIAS register configuration is based on the regulator voltage > which supplies these pbias cells, sd i/o pads. > With PBIAS register address and bit definitions different across > omap[3,4,5], Simplify PBIAS configuration under three different > regulator voltage levels - O V, 1.8 V, 3 V. Corresponding pinctrl states > are defined as pbias_off, pbias_1v8, pbias_3v. > > pinctrl state mmc_init is used for configuring speed mode, loopback clock > (in devconf0/devconf1/prog_io1 register for omap3) and pull strength > configuration (in control_mmc1 for omap4) > > Signed-off-by: Balaji T K <balajitk@ti.com> > --- > drivers/mmc/host/omap_hsmmc.c | 83 ++++++++++++++++++++++++++++++++++++++-- > 1 files changed, 78 insertions(+), 5 deletions(-) > > diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c > index 533ced2..8dd1cd3 100644 > --- a/drivers/mmc/host/omap_hsmmc.c > +++ b/drivers/mmc/host/omap_hsmmc.c > @@ -182,6 +182,9 @@ struct omap_hsmmc_host { > int req_in_progress; > struct omap_hsmmc_next next_data; > > + struct pinctrl *pinctrl; > + struct pinctrl_state *pbias_off, *pbias_1v8, *pbias_3v, *mmc_init; > + > struct omap_mmc_platform_data *pdata; > int needs_vmmc:1; > int needs_vmmc_aux:1; > @@ -267,6 +270,12 @@ static int omap_hsmmc_set_power(struct device *dev, int slot, int power_on, > if (mmc_slot(host).before_set_reg) > mmc_slot(host).before_set_reg(dev, slot, power_on, vdd); > > + if (host->pinctrl && host->pbias_off) { > + ret = pinctrl_select_state(host->pinctrl, host->pbias_off); > + if (ret < 0) > + dev_err(host->dev, "pinctrl pbias_off select error\n"); > + } > + > /* > * Assume Vcc regulator is used only to power the card ... OMAP > * VDDS is used to power the pins, optionally with a transceiver to > @@ -304,6 +313,25 @@ static int omap_hsmmc_set_power(struct device *dev, int slot, int power_on, > if (mmc_slot(host).after_set_reg) > mmc_slot(host).after_set_reg(dev, slot, power_on, vdd); > > + /* 100ms delay required for PBIAS configuration */ > + msleep(100); Hmm, why is the delay needed before the configuration? Is this something you could avoid by reading the pinconf state maybe? > + if (!vdd && host->pinctrl && host->pbias_off) { > + ret = pinctrl_select_state(host->pinctrl, host->pbias_off); > + if (ret < 0) > + dev_err(host->dev, "pinctrl pbias_off select error\n"); > + } else if (((1 << vdd) <= MMC_VDD_165_195) && host->pinctrl && > + host->pbias_1v8) { > + ret = pinctrl_select_state(host->pinctrl, host->pbias_1v8); > + if (ret < 0) > + dev_err(host->dev, "pinctrl pbias_1v8 select error\n"); > + } else if (((1 << vdd) > MMC_VDD_165_195) && host->pinctrl && > + host->pbias_3v) { > + ret = pinctrl_select_state(host->pinctrl, host->pbias_3v); > + if (ret < 0) > + dev_err(host->dev, "pinctrl pbias_3v select error\n"); > + } > + usleep_range(350, 400); > + > return ret; > } Maybe add some macro for doing the if (((1 << vdd) <= MMC_VDD_165_195)... tests? > @@ -1775,6 +1803,52 @@ static inline struct omap_mmc_platform_data > } > #endif > > +static int omap_hsmmc_pinctrl_init(struct omap_hsmmc_host *host) > +{ > + int ret = 0; > + > + host->pinctrl = devm_pinctrl_get(host->dev); > + if (IS_ERR(host->pinctrl)) { > + host->pinctrl = NULL; > + dev_warn(host->dev, > + "pins are not configured from the driver\n"); > + return 0; > + } > + > + host->mmc_init = pinctrl_lookup_state(host->pinctrl, "mmc_init"); > + if (IS_ERR(host->mmc_init)) { > + dev_vdbg(host->dev, "optional: mmc_init lookup error"); > + host->mmc_init = NULL; > + } else { > + ret = pinctrl_select_state(host->pinctrl, host->mmc_init); > + if (ret < 0) { > + dev_err(host->dev, "pinctrl mmc_init select error\n"); > + goto err_pinctrl; > + } > + } > + > + host->pbias_off = pinctrl_lookup_state(host->pinctrl, "pbias_off"); > + if (IS_ERR(host->pbias_off)) { > + dev_vdbg(host->dev, "optional: pbias_off lookup error"); > + host->pbias_off = NULL; > + } > + > + host->pbias_1v8 = pinctrl_lookup_state(host->pinctrl, "pbias_1v8"); > + if (IS_ERR(host->pbias_1v8)) { > + dev_vdbg(host->dev, "optional: pbias_1v8 lookup error"); > + host->pbias_1v8 = NULL; > + } > + > + host->pbias_3v = pinctrl_lookup_state(host->pinctrl, "pbias_3v"); > + if (IS_ERR(host->pbias_3v)) { > + dev_vdbg(host->dev, "optional: pbias_3v lookup error"); > + host->pbias_3v = NULL; > + } > + > +err_pinctrl: > + return ret; > +} Linus W may have some comments on this, although this is not the standard muxing stuff. The "mmc_init" state pins should be added to just the regular default state? Note that you can have phandles to however many sets of pins there. > static int omap_hsmmc_probe(struct platform_device *pdev) > { > struct omap_mmc_platform_data *pdata = pdev->dev.platform_data; > @@ -1785,7 +1859,6 @@ static int omap_hsmmc_probe(struct platform_device *pdev) > const struct of_device_id *match; > dma_cap_mask_t mask; > unsigned tx_req, rx_req; > - struct pinctrl *pinctrl; > > match = of_match_device(of_match_ptr(omap_mmc_of_match), &pdev->dev); > if (match) { > @@ -2005,10 +2078,9 @@ static int omap_hsmmc_probe(struct platform_device *pdev) > > omap_hsmmc_disable_irq(host); > > - pinctrl = devm_pinctrl_get_select_default(&pdev->dev); > - if (IS_ERR(pinctrl)) > - dev_warn(&pdev->dev, > - "pins are not configured from the driver\n"); > + ret = omap_hsmmc_pinctrl_init(host); > + if (ret) > + goto err_pinctrl_state; > > omap_hsmmc_protect_card(host); > > @@ -2034,6 +2106,7 @@ static int omap_hsmmc_probe(struct platform_device *pdev) > > err_slot_name: > mmc_remove_host(mmc); > +err_pinctrl_state: > free_irq(mmc_slot(host).card_detect_irq, host); > err_irq_cd: > omap_hsmmc_reg_put(host); Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 12 June 2013 08:07 PM, Tony Lindgren wrote: > * Balaji T K <balajitk@ti.com> [130606 12:20]: >> PBIAS register configuration is based on the regulator voltage >> which supplies these pbias cells, sd i/o pads. >> With PBIAS register address and bit definitions different across >> omap[3,4,5], Simplify PBIAS configuration under three different >> regulator voltage levels - O V, 1.8 V, 3 V. Corresponding pinctrl states >> are defined as pbias_off, pbias_1v8, pbias_3v. >> >> pinctrl state mmc_init is used for configuring speed mode, loopback clock >> (in devconf0/devconf1/prog_io1 register for omap3) and pull strength >> configuration (in control_mmc1 for omap4) >> >> Signed-off-by: Balaji T K <balajitk@ti.com> >> --- >> drivers/mmc/host/omap_hsmmc.c | 83 ++++++++++++++++++++++++++++++++++++++-- >> 1 files changed, 78 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c >> index 533ced2..8dd1cd3 100644 >> --- a/drivers/mmc/host/omap_hsmmc.c >> +++ b/drivers/mmc/host/omap_hsmmc.c >> @@ -182,6 +182,9 @@ struct omap_hsmmc_host { >> int req_in_progress; >> struct omap_hsmmc_next next_data; >> >> + struct pinctrl *pinctrl; >> + struct pinctrl_state *pbias_off, *pbias_1v8, *pbias_3v, *mmc_init; >> + >> struct omap_mmc_platform_data *pdata; >> int needs_vmmc:1; >> int needs_vmmc_aux:1; >> @@ -267,6 +270,12 @@ static int omap_hsmmc_set_power(struct device *dev, int slot, int power_on, >> if (mmc_slot(host).before_set_reg) >> mmc_slot(host).before_set_reg(dev, slot, power_on, vdd); >> >> + if (host->pinctrl && host->pbias_off) { >> + ret = pinctrl_select_state(host->pinctrl, host->pbias_off); >> + if (ret < 0) >> + dev_err(host->dev, "pinctrl pbias_off select error\n"); >> + } >> + >> /* >> * Assume Vcc regulator is used only to power the card ... OMAP >> * VDDS is used to power the pins, optionally with a transceiver to >> @@ -304,6 +313,25 @@ static int omap_hsmmc_set_power(struct device *dev, int slot, int power_on, >> if (mmc_slot(host).after_set_reg) >> mmc_slot(host).after_set_reg(dev, slot, power_on, vdd); >> >> + /* 100ms delay required for PBIAS configuration */ >> + msleep(100); > > Hmm, why is the delay needed before the configuration? Is this something > you could avoid by reading the pinconf state maybe? This delay is needed for regulator voltage to stabilize. > >> + if (!vdd && host->pinctrl && host->pbias_off) { >> + ret = pinctrl_select_state(host->pinctrl, host->pbias_off); >> + if (ret < 0) >> + dev_err(host->dev, "pinctrl pbias_off select error\n"); >> + } else if (((1 << vdd) <= MMC_VDD_165_195) && host->pinctrl && >> + host->pbias_1v8) { >> + ret = pinctrl_select_state(host->pinctrl, host->pbias_1v8); >> + if (ret < 0) >> + dev_err(host->dev, "pinctrl pbias_1v8 select error\n"); >> + } else if (((1 << vdd) > MMC_VDD_165_195) && host->pinctrl && >> + host->pbias_3v) { >> + ret = pinctrl_select_state(host->pinctrl, host->pbias_3v); >> + if (ret < 0) >> + dev_err(host->dev, "pinctrl pbias_3v select error\n"); >> + } >> + usleep_range(350, 400); >> + >> return ret; >> } > > Maybe add some macro for doing the if (((1 << vdd) <= MMC_VDD_165_195)... tests? OK > >> @@ -1775,6 +1803,52 @@ static inline struct omap_mmc_platform_data >> } >> #endif >> >> +static int omap_hsmmc_pinctrl_init(struct omap_hsmmc_host *host) >> +{ >> + int ret = 0; >> + >> + host->pinctrl = devm_pinctrl_get(host->dev); >> + if (IS_ERR(host->pinctrl)) { >> + host->pinctrl = NULL; >> + dev_warn(host->dev, >> + "pins are not configured from the driver\n"); >> + return 0; >> + } >> + >> + host->mmc_init = pinctrl_lookup_state(host->pinctrl, "mmc_init"); >> + if (IS_ERR(host->mmc_init)) { >> + dev_vdbg(host->dev, "optional: mmc_init lookup error"); >> + host->mmc_init = NULL; >> + } else { >> + ret = pinctrl_select_state(host->pinctrl, host->mmc_init); >> + if (ret < 0) { >> + dev_err(host->dev, "pinctrl mmc_init select error\n"); >> + goto err_pinctrl; >> + } >> + } >> + >> + host->pbias_off = pinctrl_lookup_state(host->pinctrl, "pbias_off"); >> + if (IS_ERR(host->pbias_off)) { >> + dev_vdbg(host->dev, "optional: pbias_off lookup error"); >> + host->pbias_off = NULL; >> + } >> + >> + host->pbias_1v8 = pinctrl_lookup_state(host->pinctrl, "pbias_1v8"); >> + if (IS_ERR(host->pbias_1v8)) { >> + dev_vdbg(host->dev, "optional: pbias_1v8 lookup error"); >> + host->pbias_1v8 = NULL; >> + } >> + >> + host->pbias_3v = pinctrl_lookup_state(host->pinctrl, "pbias_3v"); >> + if (IS_ERR(host->pbias_3v)) { >> + dev_vdbg(host->dev, "optional: pbias_3v lookup error"); >> + host->pbias_3v = NULL; >> + } >> + >> +err_pinctrl: >> + return ret; >> +} > > Linus W may have some comments on this, although this is not the standard > muxing stuff. The "mmc_init" state pins should be added to just the regular > default state? Note that you can have phandles to however many sets of > pins there. Yes, can add &mmc_init pin config to default state. > >> static int omap_hsmmc_probe(struct platform_device *pdev) >> { >> struct omap_mmc_platform_data *pdata = pdev->dev.platform_data; >> @@ -1785,7 +1859,6 @@ static int omap_hsmmc_probe(struct platform_device *pdev) >> const struct of_device_id *match; >> dma_cap_mask_t mask; >> unsigned tx_req, rx_req; >> - struct pinctrl *pinctrl; >> >> match = of_match_device(of_match_ptr(omap_mmc_of_match), &pdev->dev); >> if (match) { >> @@ -2005,10 +2078,9 @@ static int omap_hsmmc_probe(struct platform_device *pdev) >> >> omap_hsmmc_disable_irq(host); >> >> - pinctrl = devm_pinctrl_get_select_default(&pdev->dev); >> - if (IS_ERR(pinctrl)) >> - dev_warn(&pdev->dev, >> - "pins are not configured from the driver\n"); >> + ret = omap_hsmmc_pinctrl_init(host); >> + if (ret) >> + goto err_pinctrl_state; >> >> omap_hsmmc_protect_card(host); >> >> @@ -2034,6 +2106,7 @@ static int omap_hsmmc_probe(struct platform_device *pdev) >> >> err_slot_name: >> mmc_remove_host(mmc); >> +err_pinctrl_state: >> free_irq(mmc_slot(host).card_detect_irq, host); >> err_irq_cd: >> omap_hsmmc_reg_put(host); > > Regards, > > Tony > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 6, 2013 at 9:14 PM, Balaji T K <balajitk@ti.com> wrote: > PBIAS register configuration is based on the regulator voltage > which supplies these pbias cells, sd i/o pads. > With PBIAS register address and bit definitions different across > omap[3,4,5], Simplify PBIAS configuration under three different > regulator voltage levels - O V, 1.8 V, 3 V. Corresponding pinctrl states > are defined as pbias_off, pbias_1v8, pbias_3v. > > pinctrl state mmc_init is used for configuring speed mode, loopback clock > (in devconf0/devconf1/prog_io1 register for omap3) and pull strength > configuration (in control_mmc1 for omap4) > > Signed-off-by: Balaji T K <balajitk@ti.com> You *need* Lee Jones and Mark Brown to review this. Maybe Laurent has something to add too. Ux500 had the very same thing, and there this was solved using a GPIO regulator for "vqmmc" a level-shifter. I vaguely remember Laurent doing something similar with the SH stuff. > + /* 100ms delay required for PBIAS configuration */ > + msleep(100); > + if (!vdd && host->pinctrl && host->pbias_off) { > + ret = pinctrl_select_state(host->pinctrl, host->pbias_off); > + if (ret < 0) > + dev_err(host->dev, "pinctrl pbias_off select error\n"); > + } else if (((1 << vdd) <= MMC_VDD_165_195) && host->pinctrl && > + host->pbias_1v8) { > + ret = pinctrl_select_state(host->pinctrl, host->pbias_1v8); > + if (ret < 0) > + dev_err(host->dev, "pinctrl pbias_1v8 select error\n"); > + } else if (((1 << vdd) > MMC_VDD_165_195) && host->pinctrl && > + host->pbias_3v) { > + ret = pinctrl_select_state(host->pinctrl, host->pbias_3v); > + if (ret < 0) > + dev_err(host->dev, "pinctrl pbias_3v select error\n"); > + } So why does the pin control API control bias voltage? This seem so intuitively wrong as it can possibly get, clearly this is regulator territory. This also looks strange from an MMC point of view. It just seems these bits in these registers should be poked at by the regulator world, not the pinctrl world. That the bits are in the middle of pinctrl things does not really matter. > + usleep_range(350, 400); And the regulator framework supports power-on delays. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 12, 2013 at 4:37 PM, Tony Lindgren <tony@atomide.com> wrote: > Linus W may have some comments on this, although this is not the standard > muxing stuff. It's in the wrong subsystem and needs to be rewritten IMO. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Linus Walleij <linus.walleij@linaro.org> [130613 02:42]: > On Thu, Jun 6, 2013 at 9:14 PM, Balaji T K <balajitk@ti.com> wrote: > > > PBIAS register configuration is based on the regulator voltage > > which supplies these pbias cells, sd i/o pads. > > With PBIAS register address and bit definitions different across > > omap[3,4,5], Simplify PBIAS configuration under three different > > regulator voltage levels - O V, 1.8 V, 3 V. Corresponding pinctrl states > > are defined as pbias_off, pbias_1v8, pbias_3v. > > > > pinctrl state mmc_init is used for configuring speed mode, loopback clock > > (in devconf0/devconf1/prog_io1 register for omap3) and pull strength > > configuration (in control_mmc1 for omap4) > > > > Signed-off-by: Balaji T K <balajitk@ti.com> > > You *need* Lee Jones and Mark Brown to review this. > Maybe Laurent has something to add too. > > Ux500 had the very same thing, and there this was solved using > a GPIO regulator for "vqmmc" a level-shifter. I vaguely remember > Laurent doing something similar with the SH stuff. > > > + /* 100ms delay required for PBIAS configuration */ > > + msleep(100); > > + if (!vdd && host->pinctrl && host->pbias_off) { > > + ret = pinctrl_select_state(host->pinctrl, host->pbias_off); > > + if (ret < 0) > > + dev_err(host->dev, "pinctrl pbias_off select error\n"); > > + } else if (((1 << vdd) <= MMC_VDD_165_195) && host->pinctrl && > > + host->pbias_1v8) { > > + ret = pinctrl_select_state(host->pinctrl, host->pbias_1v8); > > + if (ret < 0) > > + dev_err(host->dev, "pinctrl pbias_1v8 select error\n"); > > + } else if (((1 << vdd) > MMC_VDD_165_195) && host->pinctrl && > > + host->pbias_3v) { > > + ret = pinctrl_select_state(host->pinctrl, host->pbias_3v); > > + if (ret < 0) > > + dev_err(host->dev, "pinctrl pbias_3v select error\n"); > > + } > > So why does the pin control API control bias voltage? I agree, it should be a regulator for the MMC driver and that's what I already suggested earlier. Having it as a regulator allows us to get rid of all the non-standard before and after calls in the omap_hsmmc.c. This way the omap_hsmmc.c code can handle the internal and external voltages the same way. > This seem so intuitively wrong as it can possibly get, clearly this > is regulator territory. The PBIAS for MMC1 is a mux + comparator for the MMC pin, so it makes sense for the regulator driver to access the register via pinctrl API. I think the reason why we have registers like this and the USB comparators in the omap SCM (System Control Module) as the all seem to relate to pin states. > This also looks strange from an MMC point of view. Yes I agree, it should be a regulator for MMC. Doing it this way just adds yet more code that's usable for only one of the omap MMC controllers. > It just seems these bits in these registers should be poked at > by the regulator world, not the pinctrl world. That the bits are > in the middle of pinctrl things does not really matter. > > > + usleep_range(350, 400); > > And the regulator framework supports power-on delays. Yes. And it seems that the delays should not be needed, but instead the comparator bits should be checked. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 13 June 2013 02:53:54 Tony Lindgren wrote: > * Linus Walleij <linus.walleij@linaro.org> [130613 02:42]: > > On Thu, Jun 6, 2013 at 9:14 PM, Balaji T K <balajitk@ti.com> wrote: > > > PBIAS register configuration is based on the regulator voltage > > > which supplies these pbias cells, sd i/o pads. > > > With PBIAS register address and bit definitions different across > > > omap[3,4,5], Simplify PBIAS configuration under three different > > > regulator voltage levels - O V, 1.8 V, 3 V. Corresponding pinctrl states > > > are defined as pbias_off, pbias_1v8, pbias_3v. > > > > > > pinctrl state mmc_init is used for configuring speed mode, loopback > > > clock (in devconf0/devconf1/prog_io1 register for omap3) and pull > > > strength configuration (in control_mmc1 for omap4) > > > > > > Signed-off-by: Balaji T K <balajitk@ti.com> > > > > You *need* Lee Jones and Mark Brown to review this. > > Maybe Laurent has something to add too. > > > > Ux500 had the very same thing, and there this was solved using > > a GPIO regulator for "vqmmc" a level-shifter. I vaguely remember > > Laurent doing something similar with the SH stuff. The SH pinctrl driver registers an MMC regulator. The code is available at git://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git. Look at drivers/pinctrl/sh-pfc/pfc-sh73a0.c in tags/renesas-next-20130611v2. > > > + /* 100ms delay required for PBIAS configuration */ > > > + msleep(100); > > > + if (!vdd && host->pinctrl && host->pbias_off) { > > > + ret = pinctrl_select_state(host->pinctrl, > > > host->pbias_off); > > > + if (ret < 0) > > > + dev_err(host->dev, "pinctrl pbias_off select > > > error\n"); + } else if (((1 << vdd) <= MMC_VDD_165_195) && > > > host->pinctrl && + host->pbias_1v8) { > > > + ret = pinctrl_select_state(host->pinctrl, > > > host->pbias_1v8); > > > + if (ret < 0) > > > + dev_err(host->dev, "pinctrl pbias_1v8 select > > > error\n"); + } else if (((1 << vdd) > MMC_VDD_165_195) && > > > host->pinctrl && + host->pbias_3v) { > > > + ret = pinctrl_select_state(host->pinctrl, > > > host->pbias_3v); > > > + if (ret < 0) > > > + dev_err(host->dev, "pinctrl pbias_3v select > > > error\n"); + } > > > > So why does the pin control API control bias voltage? > > I agree, it should be a regulator for the MMC driver and that's what I > already suggested earlier. Having it as a regulator allows us to get > rid of all the non-standard before and after calls in the omap_hsmmc.c. > This way the omap_hsmmc.c code can handle the internal and external > voltages the same way. > > > This seem so intuitively wrong as it can possibly get, clearly this > > is regulator territory. > > The PBIAS for MMC1 is a mux + comparator for the MMC pin, so it makes > sense for the regulator driver to access the register via pinctrl API. > I think the reason why we have registers like this and the USB comparators > in the omap SCM (System Control Module) as the all seem to relate to > pin states. > > > This also looks strange from an MMC point of view. > > Yes I agree, it should be a regulator for MMC. Doing it this way just > adds yet more code that's usable for only one of the omap MMC > controllers. > > > It just seems these bits in these registers should be poked at > > by the regulator world, not the pinctrl world. That the bits are > > in the middle of pinctrl things does not really matter. > > > > > + usleep_range(350, 400); > > > > And the regulator framework supports power-on delays. > > Yes. And it seems that the delays should not be needed, but instead > the comparator bits should be checked.
On Thu, 13 Jun 2013, Linus Walleij wrote: > On Thu, Jun 6, 2013 at 9:14 PM, Balaji T K <balajitk@ti.com> wrote: > > > PBIAS register configuration is based on the regulator voltage > > which supplies these pbias cells, sd i/o pads. > > With PBIAS register address and bit definitions different across > > omap[3,4,5], Simplify PBIAS configuration under three different > > regulator voltage levels - O V, 1.8 V, 3 V. Corresponding pinctrl states > > are defined as pbias_off, pbias_1v8, pbias_3v. > > > > pinctrl state mmc_init is used for configuring speed mode, loopback clock > > (in devconf0/devconf1/prog_io1 register for omap3) and pull strength > > configuration (in control_mmc1 for omap4) > > > > Signed-off-by: Balaji T K <balajitk@ti.com> > > You *need* Lee Jones and Mark Brown to review this. > Maybe Laurent has something to add too. > > Ux500 had the very same thing, and there this was solved using > a GPIO regulator for "vqmmc" a level-shifter. I vaguely remember > Laurent doing something similar with the SH stuff. I haven't seem much of this patch-set, but this certainly looks like it should be handled by a GPIO regulator instead of pinctrl. States are easily declared in a 'struct gpio_regulator_state', which the framework then uses to set the correct pins for the required voltage. And yes, 'vqmmc' is a good place to store the this regulator.
On Thu, Jun 13, 2013 at 11:53 AM, Tony Lindgren <tony@atomide.com> wrote: > * Linus Walleij <linus.walleij@linaro.org> [130613 02:42]: >> On Thu, Jun 6, 2013 at 9:14 PM, Balaji T K <balajitk@ti.com> wrote: >> > + /* 100ms delay required for PBIAS configuration */ >> > + msleep(100); >> > + if (!vdd && host->pinctrl && host->pbias_off) { >> > + ret = pinctrl_select_state(host->pinctrl, host->pbias_off); >> > + if (ret < 0) >> > + dev_err(host->dev, "pinctrl pbias_off select error\n"); >> > + } else if (((1 << vdd) <= MMC_VDD_165_195) && host->pinctrl && >> > + host->pbias_1v8) { >> > + ret = pinctrl_select_state(host->pinctrl, host->pbias_1v8); >> > + if (ret < 0) >> > + dev_err(host->dev, "pinctrl pbias_1v8 select error\n"); >> > + } else if (((1 << vdd) > MMC_VDD_165_195) && host->pinctrl && >> > + host->pbias_3v) { >> > + ret = pinctrl_select_state(host->pinctrl, host->pbias_3v); >> > + if (ret < 0) >> > + dev_err(host->dev, "pinctrl pbias_3v select error\n"); >> > + } >> >> So why does the pin control API control bias voltage? > > I agree, it should be a regulator for the MMC driver and that's what I > already suggested earlier. Having it as a regulator allows us to get > rid of all the non-standard before and after calls in the omap_hsmmc.c. > This way the omap_hsmmc.c code can handle the internal and external > voltages the same way. So someone is not taking the OMAP maintainers word for it and instead trying to push this past the pinctrl maintainer? Grrr.... OK I'll assume good faith and I therefore conclude that this must have been just some kind of mistake in the act. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 13 June 2013 03:23 PM, Tony Lindgren wrote: > * Linus Walleij <linus.walleij@linaro.org> [130613 02:42]: >> On Thu, Jun 6, 2013 at 9:14 PM, Balaji T K <balajitk@ti.com> wrote: >> >>> PBIAS register configuration is based on the regulator voltage >>> which supplies these pbias cells, sd i/o pads. >>> With PBIAS register address and bit definitions different across >>> omap[3,4,5], Simplify PBIAS configuration under three different >>> regulator voltage levels - O V, 1.8 V, 3 V. Corresponding pinctrl states >>> are defined as pbias_off, pbias_1v8, pbias_3v. >>> >>> pinctrl state mmc_init is used for configuring speed mode, loopback clock >>> (in devconf0/devconf1/prog_io1 register for omap3) and pull strength >>> configuration (in control_mmc1 for omap4) >>> >>> Signed-off-by: Balaji T K <balajitk@ti.com> >> >> You *need* Lee Jones and Mark Brown to review this. >> Maybe Laurent has something to add too. >> >> Ux500 had the very same thing, and there this was solved using >> a GPIO regulator for "vqmmc" a level-shifter. I vaguely remember >> Laurent doing something similar with the SH stuff. >> >>> + /* 100ms delay required for PBIAS configuration */ >>> + msleep(100); >>> + if (!vdd && host->pinctrl && host->pbias_off) { >>> + ret = pinctrl_select_state(host->pinctrl, host->pbias_off); >>> + if (ret < 0) >>> + dev_err(host->dev, "pinctrl pbias_off select error\n"); >>> + } else if (((1 << vdd) <= MMC_VDD_165_195) && host->pinctrl && >>> + host->pbias_1v8) { >>> + ret = pinctrl_select_state(host->pinctrl, host->pbias_1v8); >>> + if (ret < 0) >>> + dev_err(host->dev, "pinctrl pbias_1v8 select error\n"); >>> + } else if (((1 << vdd) > MMC_VDD_165_195) && host->pinctrl && >>> + host->pbias_3v) { >>> + ret = pinctrl_select_state(host->pinctrl, host->pbias_3v); >>> + if (ret < 0) >>> + dev_err(host->dev, "pinctrl pbias_3v select error\n"); >>> + } >> >> So why does the pin control API control bias voltage? > > I agree, it should be a regulator for the MMC driver and that's what I > already suggested earlier. Having it as a regulator allows us to get > rid of all the non-standard before and after calls in the omap_hsmmc.c. > This way the omap_hsmmc.c code can handle the internal and external > voltages the same way. > >> This seem so intuitively wrong as it can possibly get, clearly this >> is regulator territory. > It is not really a regulator, CONTROL_PBIAS_LITE is just a register in control module which configures pad/pin on SOC. In this case PBIAS cells are powered down before any voltage changes and after the external voltage supplied to VDDS_MMC of OMAP stabilizes pbias cells is powered ON again with specific Voltage which is given to OMAP for MMC io pins For OMAP2430, OMAP3430 It additionally has a bit for speed mode control which are set always (static config) I am quoting pbias register field description from TRM for reference BIT2 PBIASSPEEDCTRL0 Speed Control for MMC I/O 0b0 => 26 MHz I/O max speed 0b1 => 52 MHz I/O max speed BIT26 MMC1_PWRDNZ PWRDNZ control to MMC1 IO This bit is used to protect the MMC1 I/O cell when SDMMC1_VDDS is not stable. 0x0: Software must clear this bit when SDMMC1_VDDS changes. 0x1: Software must set this bit only when SDMMC1_VDDS is stable. BIT25 MMC1_PBIASLITE_HIZ_MODE HIZ_MODE from MMC1 PBIASLITE 0x0: PBIAS in normal operation mode 0x1: PBIAS output is in high impedence state BIT24 MMC1_PBIASLITE_SUPPLY_HI SUPPLY_HI_OUT from MMC1 PBIASLITE _OUT Read 0x0: SDMMC1_VDDS = 1.8V Read 0x1: SDMMC1_VDDS = 3V BIT23 MMC1_PBIASLITE_VMODE_ER VMODE ERROR from MMC1 PBIASLITE ROR Read 0x0: VMODE level is same as SUPPLY_HI_OUT Read 0x1: VMODE level is not same as SUPPLY_HI_OUT BIT22 MMC1_PBIASLITE_PWRDNZ PWRDNZ control to MMC1 PBIASLITE This bit is used to protect the MMC1_PBIAS cell (MMC1 I/O cell associated) when SDMMC1_VDDS is not stable. 0x0: Software must clear this bit when SDMMC1_VDDS changes. 0x1: Software must set this bit only when SDMMC1_VDDS is stable. BIT21 MMC1_PBIASLITE_VMODE VMODE control to MMC1 PBIASLITE 0x0: SDMMC1_VDDS = 1.8V 0x1: SDMMC1_VDDS = 3V > The PBIAS for MMC1 is a mux + comparator for the MMC pin, so it makes > sense for the regulator driver to access the register via pinctrl API. > I think the reason why we have registers like this and the USB comparators > in the omap SCM (System Control Module) as the all seem to relate to > pin states. > >> This also looks strange from an MMC point of view. > > Yes I agree, it should be a regulator for MMC. Doing it this way just > adds yet more code that's usable for only one of the omap MMC > controllers. > the only other user for pbias which I can find is USB pin configuration in arch/arm/mach-omap2/board-omap3logic.c where it is statically programmed for 3V which can be modeled as well as default pinctrl state. >> It just seems these bits in these registers should be poked at >> by the regulator world, not the pinctrl world. You mean regulator via pinctrl APIs, I think It will just move the code from omap_hsmmc to a new regulator file with it own init data for pinctrl. Not sure if Regulator maintainer will agree to it. Moreover what I needs is three different states 0V, 0 to 1.8V, 3 V to 3.3V not 0, 1.8V, 3V. plus pbias register fields got moved around between omap3, omap4 and omap5, That was one of the reason for moving to pinctrl states. >> That the bits are >> in the middle of pinctrl things does not really matter. It thought pinctrl-single,bits in pinctrl-single.c is introduced precisely for such misc control register which has bit configuration affecting different module i/o pads. >> >>> + usleep_range(350, 400); >> >> And the regulator framework supports power-on delays. > > Yes. And it seems that the delays should not be needed, but instead > the comparator bits should be checked. But, Not all OMAP has such support to read comparator bits. > > Regards, > > Tony > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 13 June 2013 03:32 PM, Laurent Pinchart wrote: > On Thursday 13 June 2013 02:53:54 Tony Lindgren wrote: >> * Linus Walleij <linus.walleij@linaro.org> [130613 02:42]: >>> On Thu, Jun 6, 2013 at 9:14 PM, Balaji T K <balajitk@ti.com> wrote: >>>> PBIAS register configuration is based on the regulator voltage >>>> which supplies these pbias cells, sd i/o pads. >>>> With PBIAS register address and bit definitions different across >>>> omap[3,4,5], Simplify PBIAS configuration under three different >>>> regulator voltage levels - O V, 1.8 V, 3 V. Corresponding pinctrl states >>>> are defined as pbias_off, pbias_1v8, pbias_3v. >>>> >>>> pinctrl state mmc_init is used for configuring speed mode, loopback >>>> clock (in devconf0/devconf1/prog_io1 register for omap3) and pull >>>> strength configuration (in control_mmc1 for omap4) >>>> >>>> Signed-off-by: Balaji T K <balajitk@ti.com> >>> >>> You *need* Lee Jones and Mark Brown to review this. >>> Maybe Laurent has something to add too. >>> >>> Ux500 had the very same thing, and there this was solved using >>> a GPIO regulator for "vqmmc" a level-shifter. I vaguely remember >>> Laurent doing something similar with the SH stuff. > > The SH pinctrl driver registers an MMC regulator. The code is available at > git://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git. Look at > drivers/pinctrl/sh-pfc/pfc-sh73a0.c in tags/renesas-next-20130611v2. > Hi, Thanks for the link, I think I need some time to understand where pfc->window[1].virt is coming from. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thursday 13 June 2013 20:22:42 Balaji T K wrote: > On Thursday 13 June 2013 03:32 PM, Laurent Pinchart wrote: > > On Thursday 13 June 2013 02:53:54 Tony Lindgren wrote: > >> * Linus Walleij <linus.walleij@linaro.org> [130613 02:42]: > >>> On Thu, Jun 6, 2013 at 9:14 PM, Balaji T K <balajitk@ti.com> wrote: > >>>> PBIAS register configuration is based on the regulator voltage > >>>> which supplies these pbias cells, sd i/o pads. > >>>> With PBIAS register address and bit definitions different across > >>>> omap[3,4,5], Simplify PBIAS configuration under three different > >>>> regulator voltage levels - O V, 1.8 V, 3 V. Corresponding pinctrl > >>>> states > >>>> are defined as pbias_off, pbias_1v8, pbias_3v. > >>>> > >>>> pinctrl state mmc_init is used for configuring speed mode, loopback > >>>> clock (in devconf0/devconf1/prog_io1 register for omap3) and pull > >>>> strength configuration (in control_mmc1 for omap4) > >>>> > >>>> Signed-off-by: Balaji T K <balajitk@ti.com> > >>> > >>> You *need* Lee Jones and Mark Brown to review this. > >>> Maybe Laurent has something to add too. > >>> > >>> Ux500 had the very same thing, and there this was solved using > >>> a GPIO regulator for "vqmmc" a level-shifter. I vaguely remember > >>> Laurent doing something similar with the SH stuff. > > > > The SH pinctrl driver registers an MMC regulator. The code is available at > > git://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git. Look at > > drivers/pinctrl/sh-pfc/pfc-sh73a0.c in tags/renesas-next-20130611v2. > > Thanks for the link, I think I need some time to understand > where pfc->window[1].virt is coming from. That's just the ioremapped pinctrl device address spaced.
On Thursday 13 June 2013 04:17 PM, Lee Jones wrote: > On Thu, 13 Jun 2013, Linus Walleij wrote: > >> On Thu, Jun 6, 2013 at 9:14 PM, Balaji T K <balajitk@ti.com> wrote: >> >>> PBIAS register configuration is based on the regulator voltage >>> which supplies these pbias cells, sd i/o pads. >>> With PBIAS register address and bit definitions different across >>> omap[3,4,5], Simplify PBIAS configuration under three different >>> regulator voltage levels - O V, 1.8 V, 3 V. Corresponding pinctrl states >>> are defined as pbias_off, pbias_1v8, pbias_3v. >>> >>> pinctrl state mmc_init is used for configuring speed mode, loopback clock >>> (in devconf0/devconf1/prog_io1 register for omap3) and pull strength >>> configuration (in control_mmc1 for omap4) >>> >>> Signed-off-by: Balaji T K <balajitk@ti.com> >> >> You *need* Lee Jones and Mark Brown to review this. >> Maybe Laurent has something to add too. >> >> Ux500 had the very same thing, and there this was solved using >> a GPIO regulator for "vqmmc" a level-shifter. I vaguely remember >> Laurent doing something similar with the SH stuff. > > I haven't seem much of this patch-set, but this certainly looks like > it should be handled by a GPIO regulator instead of pinctrl. States > are easily declared in a 'struct gpio_regulator_state', which the > framework then uses to set the correct pins for the required voltage. > Thanks for the pointer, but wondering why is it named as gpio-regulator and how it is different from fixed-regulator. After going through git log description, I understand that voltage/current level for a particular regulator is controlled by a set of pad/pin on the POWER IC and pad/pin may be usually connected to gpio pins if it is needs to be configurable and ground/pulled for constant voltage. Collection of gpios logic level are modeled as state for particular voltage. But gpio is not used in my case. > And yes, 'vqmmc' is a good place to store the this regulator. > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 13, 2013 at 4:41 PM, Balaji T K <balajitk@ti.com> wrote: >[Me] >>> This seem so intuitively wrong as it can possibly get, clearly this >>> is regulator territory. > > It is not really a regulator, CONTROL_PBIAS_LITE is just a register > in control module which configures pad/pin on SOC. In this case PBIAS cells > are powered down before any voltage changes and after the external voltage > supplied to VDDS_MMC of OMAP stabilizes pbias cells is powered ON again > with specific Voltage which is given to OMAP for MMC io pins So there is some external actual regulator supplying the voltage and then that is looped back in to the pads. (I understand this is quite a common construction.) Anyway since it is obviously removing and applying voltage to the pins, I think this is more of a cascaded regulator. We use regulators for simple light-switches and things you know. We have generic pinconf options for things like current but not voltage really. It's because in the case of pin current configuration it's basically about how many driver stages (totempoles) you connect to the output, which is very different from the voltages we're dealing with here. So I think these bits go into a regulator driver: > BIT26 MMC1_PWRDNZ PWRDNZ control to MMC1 IO > This bit is used to protect the MMC1 I/O cell when > SDMMC1_VDDS is not stable. > 0x0: Software must clear this bit when SDMMC1_VDDS > changes. > 0x1: Software must set this bit only when > SDMMC1_VDDS is stable. > > BIT24 MMC1_PBIASLITE_SUPPLY_HI SUPPLY_HI_OUT from MMC1 PBIASLITE > _OUT Read 0x0: SDMMC1_VDDS = 1.8V > Read 0x1: SDMMC1_VDDS = 3V > > BIT23 MMC1_PBIASLITE_VMODE_ER VMODE ERROR from MMC1 PBIASLITE > ROR Read 0x0: VMODE level is same as SUPPLY_HI_OUT > Read 0x1: VMODE level is not same as > SUPPLY_HI_OUT > > BIT22 MMC1_PBIASLITE_PWRDNZ PWRDNZ control to MMC1 PBIASLITE > This bit is used to protect the MMC1_PBIAS cell (MMC1 > I/O cell associated) when SDMMC1_VDDS is not stable. > 0x0: Software must clear this bit when SDMMC1_VDDS > changes. > 0x1: Software must set this bit only when > SDMMC1_VDDS is stable. > > BIT21 MMC1_PBIASLITE_VMODE VMODE control to MMC1 PBIASLITE > 0x0: SDMMC1_VDDS = 1.8V > 0x1: SDMMC1_VDDS = 3V This looks much more cascaded regulator control than anything else to me. > For OMAP2430, OMAP3430 It additionally has a bit for speed mode control > which are set always (static config) I guess you mean this: > BIT2 PBIASSPEEDCTRL0 Speed Control for MMC I/O > 0b0 => 26 MHz I/O max speed > 0b1 => 52 MHz I/O max speed This seems to belong to the MMC host driver. It is common that registers contain indiviual bits that end up in different subsystems. Speed mode seems to belong in the MMC driver. The infrastructure used to spread out responsibility across different drivers from a common register range or even for certain bits in a certain register, is regmap. Check for example mfd/syscon.c. > BIT25 MMC1_PBIASLITE_HIZ_MODE HIZ_MODE from MMC1 PBIASLITE > 0x0: PBIAS in normal operation mode > 0x1: PBIAS output is in high impedence state This is actually pin config. But if it makes sense it should also be part of the MMC regulator driver. > You mean regulator via pinctrl APIs, I think It will just move the code > from omap_hsmmc to a new regulator file with it own init data for pinctrl. No I'm not saying you should use pinctrl as a "back-end" for this. I mean you shall instantiate a regulator and let the callback ops vtable for that regulator poke these bits. > Not sure if Regulator maintainer will agree to it. I will respect Marks judgement on this for sure. > Moreover what I needs is three different states 0V, 0 to 1.8V, 3 V to 3.3V > not 0, 1.8V, 3V. plus pbias register fields got moved around between omap3, > omap4 > and omap5, That was one of the reason for moving to pinctrl states. The regulator framework supports selector tables just like this. Where the register fields are located does not matter. >>> That the bits are >>> in the middle of pinctrl things does not really matter. > > It thought pinctrl-single,bits in pinctrl-single.c is introduced > precisely for such misc control register which has bit configuration > affecting different module i/o pads. No. If we go down that road *anything* that is connected to a pad becomes part of the pinctrl subsystem, then pinctrl-single becomes some kind of hardware abstraction or BIOS, and that is *not* the intent. It is only supposed to deal with the bits there that are 100% related to what pinctrl does, nothing else. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 13 Jun 2013, Balaji T K wrote: > On Thursday 13 June 2013 04:17 PM, Lee Jones wrote: > >On Thu, 13 Jun 2013, Linus Walleij wrote: > > > >>On Thu, Jun 6, 2013 at 9:14 PM, Balaji T K <balajitk@ti.com> wrote: > >> > >>>PBIAS register configuration is based on the regulator voltage > >>>which supplies these pbias cells, sd i/o pads. > >>>With PBIAS register address and bit definitions different across > >>>omap[3,4,5], Simplify PBIAS configuration under three different > >>>regulator voltage levels - O V, 1.8 V, 3 V. Corresponding pinctrl states > >>>are defined as pbias_off, pbias_1v8, pbias_3v. > >>> > >>>pinctrl state mmc_init is used for configuring speed mode, loopback clock > >>>(in devconf0/devconf1/prog_io1 register for omap3) and pull strength > >>>configuration (in control_mmc1 for omap4) > >>> > >>>Signed-off-by: Balaji T K <balajitk@ti.com> > >> > >>You *need* Lee Jones and Mark Brown to review this. > >>Maybe Laurent has something to add too. > >> > >>Ux500 had the very same thing, and there this was solved using > >>a GPIO regulator for "vqmmc" a level-shifter. I vaguely remember > >>Laurent doing something similar with the SH stuff. > > > >I haven't seem much of this patch-set, but this certainly looks like > >it should be handled by a GPIO regulator instead of pinctrl. States > >are easily declared in a 'struct gpio_regulator_state', which the > >framework then uses to set the correct pins for the required voltage. > > > > Thanks for the pointer, but wondering why is it named as gpio-regulator > and how it is different from fixed-regulator. > After going through git log description, I understand that voltage/current level > for a particular regulator is controlled by a set of pad/pin on the POWER IC > and pad/pin may be usually connected to gpio pins if it is needs to be > configurable and ground/pulled for constant voltage. > > Collection of gpios logic level are modeled as state for particular voltage. > But gpio is not used in my case. > > >And yes, 'vqmmc' is a good place to store the this regulator. As I say, I didn't see much of the code, only parts which looked similar a voltage level-shifter. The difference between fixed and gpio regulators, is that the former is exactly that, 'fixed'. You can turn voltage on and off using a gpio pin, but you can't shift the voltage. Something which is required of your use-case. The latter switches between voltgages via a set of gpio pins, for instance, your use-case could look somelike like: static struct gpio_regulator_state sdi0_reg_states[] = { { .value = 3300000, .gpios = (1 << 0) }, { .value = 1800000, .gpios = (0 << 0) }, }; But if there aren't any gpio pins involved, then this isn't what you want either.
* Linus Walleij <linus.walleij@linaro.org> [130613 08:35]: > On Thu, Jun 13, 2013 at 4:41 PM, Balaji T K <balajitk@ti.com> wrote: > > > You mean regulator via pinctrl APIs, I think It will just move the code > > from omap_hsmmc to a new regulator file with it own init data for pinctrl. > > No I'm not saying you should use pinctrl as a "back-end" for this. > I mean you shall instantiate a regulator and let the callback ops > vtable for that regulator poke these bits. The interface to omap_hsmmc.c should be the regulator framework. This is because it allows us to clean up all the messed up before and after functions that really implement various GPIO regulators etc. > > It thought pinctrl-single,bits in pinctrl-single.c is introduced > > precisely for such misc control register which has bit configuration > > affecting different module i/o pads. > > No. If we go down that road *anything* that is connected to a > pad becomes part of the pinctrl subsystem, then pinctrl-single > becomes some kind of hardware abstraction or BIOS, and that > is *not* the intent. It is only supposed to deal with the bits > there that are 100% related to what pinctrl does, nothing else. Sounds like the way to go is to do a standalone regulator driver that optionally uses pinctrl-single,bits. But only for the bits in the PBIAS register that are 100% related to pinctrl. In any case the PBIAS regulator driver should be a separate driver as it may need to be a child of the SCM driver for PM needs in the future. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 13, 2013 at 09:29:38AM -0700, Tony Lindgren wrote: > * Linus Walleij <linus.walleij@linaro.org> [130613 08:35]: > > No. If we go down that road *anything* that is connected to a > > pad becomes part of the pinctrl subsystem, then pinctrl-single > > becomes some kind of hardware abstraction or BIOS, and that > > is *not* the intent. It is only supposed to deal with the bits > > there that are 100% related to what pinctrl does, nothing else. > Sounds like the way to go is to do a standalone regulator driver that > optionally uses pinctrl-single,bits. But only for the bits in the PBIAS > register that are 100% related to pinctrl. > In any case the PBIAS regulator driver should be a separate driver > as it may need to be a child of the SCM driver for PM needs in the > future. This all seems sensible to me.
Few cleanups to reduce code indent, Add pbias_regulator support and adapt omap_hsmmc to use pbias regulator to configure required voltage on mmc1 pad(SD card) i/o rails on OMAP SoCs. Balaji T K (8): mmc: omap_hsmmc: use devm_regulator API mmc: omap_hsmmc: handle vcc and vcc_aux independently regulator: add pbias regulator support mmc: omap_hsmmc: adapt hsmmc to use pbias regulator ARM: dts: add pbias dt node ARM: dts: add pbias-supply ARM: OMAP: enable SYSCON and REGULATOR_PBIAS in omap2plus_defconfig mmc: omap_hsmmc: remove pbias workaround .../bindings/regulator/pbias-regulator.txt | 21 ++ arch/arm/boot/dts/dra7-evm.dts | 1 + arch/arm/boot/dts/dra7.dtsi | 12 + arch/arm/boot/dts/omap2430.dtsi | 12 + arch/arm/boot/dts/omap3-beagle-xm.dts | 1 + arch/arm/boot/dts/omap3-beagle.dts | 1 + arch/arm/boot/dts/omap3-devkit8000.dts | 1 + arch/arm/boot/dts/omap3-evm-common.dtsi | 1 + arch/arm/boot/dts/omap3-gta04.dts | 1 + arch/arm/boot/dts/omap3-igep.dtsi | 1 + arch/arm/boot/dts/omap3-n900.dts | 1 + arch/arm/boot/dts/omap3-overo.dtsi | 1 + arch/arm/boot/dts/omap3-zoom3.dts | 1 + arch/arm/boot/dts/omap3.dtsi | 12 + arch/arm/boot/dts/omap3430-sdp.dts | 1 + arch/arm/boot/dts/omap4-panda-common.dtsi | 1 + arch/arm/boot/dts/omap4-sdp.dts | 1 + arch/arm/boot/dts/omap4-var-som.dts | 1 + arch/arm/boot/dts/omap4.dtsi | 12 + arch/arm/boot/dts/omap5-uevm.dts | 1 + arch/arm/boot/dts/omap5.dtsi | 12 + arch/arm/configs/omap2plus_defconfig | 2 + drivers/mmc/host/omap_hsmmc.c | 113 +++++---- drivers/regulator/Kconfig | 7 + drivers/regulator/Makefile | 1 + drivers/regulator/pbias-regulator.c | 264 ++++++++++++++++++++ 26 files changed, 434 insertions(+), 49 deletions(-) create mode 100644 Documentation/devicetree/bindings/regulator/pbias-regulator.txt create mode 100644 drivers/regulator/pbias-regulator.c
Few cleanups to reduce code indent, Add pbias_regulator support and adapt omap_hsmmc to use pbias regulator to configure required voltage on mmc1 pad(SD card) i/o rails on OMAP SoCs. Balaji T K (7): mmc: omap_hsmmc: use devm_regulator API mmc: omap_hsmmc: handle vcc and vcc_aux independently regulator: add pbias regulator support mmc: omap_hsmmc: adapt hsmmc to use pbias regulator ARM: dts: add pbias dt node ARM: OMAP: enable SYSCON and REGULATOR_PBIAS in omap2plus_defconfig mmc: omap_hsmmc: remove pbias workaround .../bindings/regulator/pbias-regulator.txt | 21 ++ arch/arm/boot/dts/dra7.dtsi | 14 ++ arch/arm/boot/dts/omap2430.dtsi | 14 ++ arch/arm/boot/dts/omap3.dtsi | 14 ++ arch/arm/boot/dts/omap4.dtsi | 14 ++ arch/arm/boot/dts/omap5.dtsi | 14 ++ arch/arm/configs/omap2plus_defconfig | 2 + drivers/mmc/host/omap_hsmmc.c | 113 ++++++---- drivers/regulator/Kconfig | 9 + drivers/regulator/Makefile | 1 + drivers/regulator/pbias-regulator.c | 232 ++++++++++++++++++++ 11 files changed, 399 insertions(+), 49 deletions(-) create mode 100644 Documentation/devicetree/bindings/regulator/pbias-regulator.txt create mode 100644 drivers/regulator/pbias-regulator.c
Few cleanups to reduce code indent, Add pbias_regulator support and adapt omap_hsmmc to use pbias regulator to configure required voltage on mmc1 pad(SD card) i/o rails on OMAP SoCs. Balaji T K (7): mmc: omap_hsmmc: use devm_regulator API mmc: omap_hsmmc: handle vcc and vcc_aux independently regulator: add pbias regulator support mmc: omap_hsmmc: adapt hsmmc to use pbias regulator ARM: dts: add pbias dt node ARM: OMAP: enable SYSCON and REGULATOR_PBIAS in omap2plus_defconfig mmc: omap_hsmmc: remove pbias workaround .../bindings/regulator/pbias-regulator.txt | 17 ++ arch/arm/boot/dts/dra7.dtsi | 19 ++ arch/arm/boot/dts/omap2430.dtsi | 19 ++ arch/arm/boot/dts/omap3.dtsi | 19 ++ arch/arm/boot/dts/omap4.dtsi | 19 ++ arch/arm/boot/dts/omap5.dtsi | 19 ++ arch/arm/configs/omap2plus_defconfig | 2 + drivers/mmc/host/omap_hsmmc.c | 113 +++++---- drivers/regulator/Kconfig | 9 + drivers/regulator/Makefile | 1 + drivers/regulator/pbias-regulator.c | 255 ++++++++++++++++++++ 11 files changed, 443 insertions(+), 49 deletions(-) create mode 100644 Documentation/devicetree/bindings/regulator/pbias-regulator.txt create mode 100644 drivers/regulator/pbias-regulator.c
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c index 533ced2..8dd1cd3 100644 --- a/drivers/mmc/host/omap_hsmmc.c +++ b/drivers/mmc/host/omap_hsmmc.c @@ -182,6 +182,9 @@ struct omap_hsmmc_host { int req_in_progress; struct omap_hsmmc_next next_data; + struct pinctrl *pinctrl; + struct pinctrl_state *pbias_off, *pbias_1v8, *pbias_3v, *mmc_init; + struct omap_mmc_platform_data *pdata; int needs_vmmc:1; int needs_vmmc_aux:1; @@ -267,6 +270,12 @@ static int omap_hsmmc_set_power(struct device *dev, int slot, int power_on, if (mmc_slot(host).before_set_reg) mmc_slot(host).before_set_reg(dev, slot, power_on, vdd); + if (host->pinctrl && host->pbias_off) { + ret = pinctrl_select_state(host->pinctrl, host->pbias_off); + if (ret < 0) + dev_err(host->dev, "pinctrl pbias_off select error\n"); + } + /* * Assume Vcc regulator is used only to power the card ... OMAP * VDDS is used to power the pins, optionally with a transceiver to @@ -304,6 +313,25 @@ static int omap_hsmmc_set_power(struct device *dev, int slot, int power_on, if (mmc_slot(host).after_set_reg) mmc_slot(host).after_set_reg(dev, slot, power_on, vdd); + /* 100ms delay required for PBIAS configuration */ + msleep(100); + if (!vdd && host->pinctrl && host->pbias_off) { + ret = pinctrl_select_state(host->pinctrl, host->pbias_off); + if (ret < 0) + dev_err(host->dev, "pinctrl pbias_off select error\n"); + } else if (((1 << vdd) <= MMC_VDD_165_195) && host->pinctrl && + host->pbias_1v8) { + ret = pinctrl_select_state(host->pinctrl, host->pbias_1v8); + if (ret < 0) + dev_err(host->dev, "pinctrl pbias_1v8 select error\n"); + } else if (((1 << vdd) > MMC_VDD_165_195) && host->pinctrl && + host->pbias_3v) { + ret = pinctrl_select_state(host->pinctrl, host->pbias_3v); + if (ret < 0) + dev_err(host->dev, "pinctrl pbias_3v select error\n"); + } + usleep_range(350, 400); + return ret; } @@ -1775,6 +1803,52 @@ static inline struct omap_mmc_platform_data } #endif +static int omap_hsmmc_pinctrl_init(struct omap_hsmmc_host *host) +{ + int ret = 0; + + host->pinctrl = devm_pinctrl_get(host->dev); + if (IS_ERR(host->pinctrl)) { + host->pinctrl = NULL; + dev_warn(host->dev, + "pins are not configured from the driver\n"); + return 0; + } + + host->mmc_init = pinctrl_lookup_state(host->pinctrl, "mmc_init"); + if (IS_ERR(host->mmc_init)) { + dev_vdbg(host->dev, "optional: mmc_init lookup error"); + host->mmc_init = NULL; + } else { + ret = pinctrl_select_state(host->pinctrl, host->mmc_init); + if (ret < 0) { + dev_err(host->dev, "pinctrl mmc_init select error\n"); + goto err_pinctrl; + } + } + + host->pbias_off = pinctrl_lookup_state(host->pinctrl, "pbias_off"); + if (IS_ERR(host->pbias_off)) { + dev_vdbg(host->dev, "optional: pbias_off lookup error"); + host->pbias_off = NULL; + } + + host->pbias_1v8 = pinctrl_lookup_state(host->pinctrl, "pbias_1v8"); + if (IS_ERR(host->pbias_1v8)) { + dev_vdbg(host->dev, "optional: pbias_1v8 lookup error"); + host->pbias_1v8 = NULL; + } + + host->pbias_3v = pinctrl_lookup_state(host->pinctrl, "pbias_3v"); + if (IS_ERR(host->pbias_3v)) { + dev_vdbg(host->dev, "optional: pbias_3v lookup error"); + host->pbias_3v = NULL; + } + +err_pinctrl: + return ret; +} + static int omap_hsmmc_probe(struct platform_device *pdev) { struct omap_mmc_platform_data *pdata = pdev->dev.platform_data; @@ -1785,7 +1859,6 @@ static int omap_hsmmc_probe(struct platform_device *pdev) const struct of_device_id *match; dma_cap_mask_t mask; unsigned tx_req, rx_req; - struct pinctrl *pinctrl; match = of_match_device(of_match_ptr(omap_mmc_of_match), &pdev->dev); if (match) { @@ -2005,10 +2078,9 @@ static int omap_hsmmc_probe(struct platform_device *pdev) omap_hsmmc_disable_irq(host); - pinctrl = devm_pinctrl_get_select_default(&pdev->dev); - if (IS_ERR(pinctrl)) - dev_warn(&pdev->dev, - "pins are not configured from the driver\n"); + ret = omap_hsmmc_pinctrl_init(host); + if (ret) + goto err_pinctrl_state; omap_hsmmc_protect_card(host); @@ -2034,6 +2106,7 @@ static int omap_hsmmc_probe(struct platform_device *pdev) err_slot_name: mmc_remove_host(mmc); +err_pinctrl_state: free_irq(mmc_slot(host).card_detect_irq, host); err_irq_cd: omap_hsmmc_reg_put(host);
PBIAS register configuration is based on the regulator voltage which supplies these pbias cells, sd i/o pads. With PBIAS register address and bit definitions different across omap[3,4,5], Simplify PBIAS configuration under three different regulator voltage levels - O V, 1.8 V, 3 V. Corresponding pinctrl states are defined as pbias_off, pbias_1v8, pbias_3v. pinctrl state mmc_init is used for configuring speed mode, loopback clock (in devconf0/devconf1/prog_io1 register for omap3) and pull strength configuration (in control_mmc1 for omap4) Signed-off-by: Balaji T K <balajitk@ti.com> --- drivers/mmc/host/omap_hsmmc.c | 83 ++++++++++++++++++++++++++++++++++++++-- 1 files changed, 78 insertions(+), 5 deletions(-)