From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Kocialkowski Date: Sun, 02 Nov 2014 20:01:56 +0100 Subject: [U-Boot] [PATCH v3 2/2] omap_hsmmc: Board-specific TWL4030 MMC power initializations In-Reply-To: <54563430.30309@compulab.co.il> References: <1414839138-6587-1-git-send-email-contact@paulk.fr> <1414839138-6587-3-git-send-email-contact@paulk.fr> <54563430.30309@compulab.co.il> Message-ID: <1414954916.17533.13.camel@collins> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > > 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: