Re: [PATCH 06/13] mmc: omap_hsmmc: add dt pbias and control mmc support

[Tony Lindgren <tony@atomide.com>]

Hi,

* Balaji T K <balajitk@ti.com> [130430 07:09]:
> Add omap_hsmmc_control to support pbias, high speed mode configuration for mmc1,
> loopback clock configuration (when external transceiver is used) for mmc2

Thanks for working on this, few suggestions inlined below.
 
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -182,6 +182,7 @@ struct omap_hsmmc_host {
>  	struct omap_hsmmc_next	next_data;
>  
>  	struct	omap_mmc_platform_data	*pdata;
> +	struct omap_hsmmc_control	*ctrl_mmc;
>  	int needs_vmmc:1;
>  	int needs_vmmc_aux:1;
>  };
> @@ -265,6 +266,8 @@ 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->ctrl_mmc && host->ctrl_mmc->before)
> +		host->ctrl_mmc->before(host->ctrl_mmc->dev, power_on, vdd);
>  
>  	/*
>  	 * Assume Vcc regulator is used only to power the card ... OMAP
> @@ -302,6 +305,8 @@ 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);
> +	if (host->ctrl_mmc && host->ctrl_mmc->after)
> +		host->ctrl_mmc->after(host->ctrl_mmc->dev, power_on, vdd);
>  
>  	return ret;
>  }

These before and after functions should be first converted to just usual
regulator_set_voltage() eventually. In the PBIAS case it's really a mux
plus a comparator, but we can set it up as a regulator. And on some boards
it can be an external regulator like we have the legacy callbacks for in
mach-omap2/hsmmc.c.

> +++ b/drivers/mmc/host/omap_hsmmc_control.c
> @@ -0,0 +1,466 @@
> +static void omap_control_mmc_writel(u32 reg, u32 *base2)
> +{
> +	if (base2)
> +		__raw_writel(reg, base2);
> +	return;
> +}
> +
> +static u32 omap_control_mmc_readl(u32 *base2)
> +{
> +	u32 pbias_reg = 0;
> +
> +	if (base2)
> +		pbias_reg = __raw_readl(base2);
> +	return pbias_reg;
> +}
> +
> +static void omap2430_mmc1_active_overwrite(u32 __iomem *devconf1, int vdd)
> +{
> +	u32 reg;
> +
> +	reg = omap_control_mmc_readl(devconf1);
> +	if ((1 << vdd) >= MMC_VDD_30_31)
> +		reg |= OMAP243X_MMC1_ACTIVE_OVERWRITE;
> +	else
> +		reg &= ~OMAP243X_MMC1_ACTIVE_OVERWRITE;
> +	omap_control_mmc_writel(reg, devconf1);
> +}
> +/* pbias configuration for omap2430, omap3 */
> +static void omap_hsmmc1_before_set_reg(struct device *dev,
> +				  int power_on, int vdd)
> +{
> +	u32 reg;
> +	struct omap_hsmmc_control *ctl_mmc = dev_get_drvdata(dev);
> +
> +	if (ctl_mmc->devconf1)
> +		omap2430_mmc1_active_overwrite(ctl_mmc->devconf1, vdd);
> +
> +	reg = omap_control_mmc_readl(ctl_mmc->pbias);
> +	reg &= ~OMAP2_PBIASLITEPWRDNZ0;
> +	omap_control_mmc_writel(reg, ctl_mmc->pbias);
> +}
> +
> +static void omap_hsmmc1_after_set_reg(struct device *dev,
> +				 int power_on, int vdd)
> +{
> +	u32 reg;
> +	struct omap_hsmmc_control *ctl_mmc = dev_get_drvdata(dev);
> +
> +	/* 100ms delay required for PBIAS configuration */
> +	msleep(100);
> +
> +	if (power_on) {
> +		reg = omap_control_mmc_readl(ctl_mmc->pbias);
> +		reg |= OMAP2_PBIASLITEPWRDNZ0;
> +		if ((1 << vdd) <= MMC_VDD_165_195)
> +			reg &= ~OMAP2_PBIASLITEVMODE0;
> +		else
> +			reg |= OMAP2_PBIASLITEVMODE0;
> +		omap_control_mmc_writel(reg, ctl_mmc->pbias);
> +	} else {
> +		reg = omap_control_mmc_readl(ctl_mmc->pbias);
> +		reg |= (OMAP2_PBIASLITEPWRDNZ0 |
> +			OMAP2_PBIASLITEVMODE0);
> +		omap_control_mmc_writel(reg, ctl_mmc->pbias);
> +	}
> +}
...

This all we can simplify quite a bit by defining the PBIAS register
as pinctrl-single,bits with two different named modes. One for
1.8V and for 3.3V. This way the PBIAS register is abstracted for
various omaps in the .dts file as the register is different.

Then this file should just define a new regulator that requests the
defined pinctrl named mode with pinctrl_select_state().

Now the only thing missing AFAIK is getting the comparator value
for checks with the generic pinconf framework. But you can already
get the raw register value with pin_config_get() and add the
checking to this driver until pinconf allows us to do that.

BTW, the same can then be done for the USB transceivers if we
figure out a way to properly deal with comparators with generic
pinconf.

Regards,

Tony