[U-Boot] [PATCH v3 2/2] omap_hsmmc: Board-specific TWL4030 MMC power initializations
Paul Kocialkowski
contact at paulk.fr
Sun Nov 2 20:01:56 CET 2014
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 at 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>
More information about the U-Boot
mailing list