[U-Boot] [PATCH v3 2/2] omap_hsmmc: Board-specific TWL4030 MMC power initializations

[Paul Kocialkowski <contact@paulk.fr>]

Hi Igor, thanks for the review.

> On 11/01/14 12:52, Paul Kocialkowski wrote:
> > Boards using the TWL4030 regulator may not all use the LDOs the same way
> > (e.g. MMC2 power can be controlled by another LDO than VMMC2).
> > This delegates TWL4030 MMC power initializations to board-specific functions,
> > that may still call twl4030_power_mmc_init for the default behavior.
> > 
> > Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> 
> This mostly looks good, several suggestions below though.
> 
> > ---
> >  board/comelit/dig297/dig297.c          |  9 +++++++++
> >  board/compulab/cm_t35/cm_t35.c         | 11 +++++++++++
> >  board/corscience/tricorder/tricorder.c | 11 +++++++++++
> >  board/isee/igep00x0/igep00x0.c         | 11 +++++++++++
> >  board/logicpd/omap3som/omap3logic.c    | 11 +++++++++++
> >  board/logicpd/zoom1/zoom1.c            |  9 +++++++++
> >  board/matrix_vision/mvblx/mvblx.c      |  9 +++++++++
> >  board/nokia/rx51/rx51.c                |  9 +++++++++
> >  board/overo/overo.c                    | 11 +++++++++++
> >  board/pandora/pandora.c                |  9 +++++++++
> >  board/technexion/tao3530/tao3530.c     | 11 +++++++++++
> >  board/ti/beagle/beagle.c               | 11 +++++++++++
> >  board/ti/evm/evm.c                     | 11 +++++++++++
> >  board/ti/sdp3430/sdp.c                 |  9 +++++++++
> >  board/timll/devkit8000/devkit8000.c    | 11 +++++++++++
> >  drivers/mmc/omap_hsmmc.c               |  7 +------
> >  16 files changed, 154 insertions(+), 6 deletions(-)
> > 
> > diff --git a/board/comelit/dig297/dig297.c b/board/comelit/dig297/dig297.c
> > index 2b826df..784483b 100644
> > --- a/board/comelit/dig297/dig297.c
> > +++ b/board/comelit/dig297/dig297.c
> > @@ -133,6 +133,15 @@ int board_mmc_init(bd_t *bis)
> >  {
> >  	return omap_mmc_init(0, 0, 0, -1, -1);
> >  }
> > +
> > +int board_mmc_power_init(void)
> > +{
> > +#if defined(CONFIG_TWL4030_POWER)
> > +	twl4030_power_mmc_init();
> > +	mdelay(100);	/* ramp-up delay from Linux code */
> 
> I guess all twl4030 based boards have to have this ramp up delay, right?
> If so, why don't we want to hide it inside the twl4030_power_mmc_init()
> function and not spread it across all boards?

That makes perfect sense, thanks for the suggestion. Will do in the next
version.

> > +#endif
> > +	return 0;
> > +}
> 
> Also, what do you think of the below suggestion:
> Leave the twl4030_power_mmc_init() call inside the omap_hsmmc.c
> (as it seems that many boards want it) as it currently is.
> This way you will not need to patch each board file, yet a board
> has the ability to control that call via the CONFIG_TWL4030_POWER
> and have a board specific callback should it need one.

One of the initial intents of this patch set was also to remove the
implication of CONFIG_TWL4030_POWER on twl4030_power_mmc_init, because
my device (LG Optimus Black P970) uses TWL4030 regulators in a way that
doesn't match other boards or any reference design (so I need
CONFIG_TWL4030_POWER but not twl4030_power_mmc_init).

> I would also change envelope the call to twl4030_power_mmc_init()
> to something like CONFIG_TWL4030_MMC_POWER instead of
> CONFIG_TWL4030_POWER, so it will be more flexible, but that has
> little to do with this patch.

That would be acceptable for me, but it would mean having two places
where the same thing happens: the new board_mmc_power_init function and
internally on omap_hsmmc. That's the sort of lack of consistance I
definitely do not like, but if you think it's the best way to do it, I
don't see any further issue with that.

Patching all the boards isn't really that big a deal (identifying the
boards is already done now anyway) and it makes similar sense to having
multiple board_mmc_init that all look very much alike.

In addition, perhaps I should add a dev_index parameter to
twl4030_power_mmc_init (in a separate patch) so that only the relevant
LDO is enabled (see my previous patch enabling VMMC2 too, that was
accepted): most devices only use one MMC controller, so there is no need
to enable all the MMC LDOs.

That would give some legitimacy to having one twl4030_power_mmc_init per
board, since it would reflect whether to enable one or both MMC LDOs.

What do you think?

> >  #endif
> >  
> >  #ifdef CONFIG_CMD_NET
> 
> [...]
> 
> > diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c
> > index ef2cbf9..6fb78b3 100644
> > --- a/drivers/mmc/omap_hsmmc.c
> > +++ b/drivers/mmc/omap_hsmmc.c
> > @@ -135,12 +135,7 @@ static unsigned char mmc_board_init(struct mmc *mmc)
> >  	pbias_lite = readl(&t2_base->pbias_lite);
> >  	pbias_lite &= ~(PBIASLITEPWRDNZ1 | PBIASLITEPWRDNZ0);
> >  	writel(pbias_lite, &t2_base->pbias_lite);
> > -#endif
> > -#if defined(CONFIG_TWL4030_POWER)
> > -	twl4030_power_mmc_init();
> > -	mdelay(100);	/* ramp-up delay from Linux code */
> > -#endif
> > -#if defined(CONFIG_OMAP34XX)
> > +
> >  	writel(pbias_lite | PBIASLITEPWRDNZ1 |
> >  		PBIASSPEEDCTRL0 | PBIASLITEPWRDNZ0,
> >  		&t2_base->pbias_lite);
> > 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20141102/de1c266e/attachment.pgp>