[PATCH v2 06/10] mmc: omap_hsmmc: add support for pbias configuration in dt

[Laurent Pinchart]

On Thursday 13 June 2013 02:53:54 Tony Lindgren wrote:
> * Linus Walleij <linus.walleij at linaro.org> [130613 02:42]:
> > On Thu, Jun 6, 2013 at 9:14 PM, Balaji T K <balajitk at 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 at 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.

-- 
Regards,

Laurent Pinchart